From 9716ed38d193792cf04c2dd945d9c7d85141d0a1 Mon Sep 17 00:00:00 2001 From: Troels Liebe Bentsen Date: Mon, 18 May 2020 21:47:49 +0200 Subject: [PATCH] Fix more TODOs --- cmd/authwrapper/main.go | 1 - cmd/authwrapper/setup.go | 21 ++++++++++++++++----- kms/google/kms-signer.go | 2 -- server/main.go | 21 ++++++++++++++++++--- server/utils.go | 19 ++++++++++++++++--- 5 files changed, 50 insertions(+), 14 deletions(-) diff --git a/cmd/authwrapper/main.go b/cmd/authwrapper/main.go index 07cd8be..eb4d500 100644 --- a/cmd/authwrapper/main.go +++ b/cmd/authwrapper/main.go @@ -15,7 +15,6 @@ func main() { log.Fatalf(": %v", err) } - // TODO: Default to port if nothing has been set if config.SSHCaKeyPath != "" && config.SSHSigningServerAddress != "" { caPublickey, err := startSigningServer( config.SSHCaKeyPath, diff --git a/cmd/authwrapper/setup.go b/cmd/authwrapper/setup.go index 8683bbb..211357c 100644 --- a/cmd/authwrapper/setup.go +++ b/cmd/authwrapper/setup.go @@ -39,7 +39,7 @@ func parseEnvironment() (*Config, error) { config := &Config{ Command: os.Getenv("WRAP_COMMAND"), Args: os.Args[1:], - RequestedPrincipals: strings.Split(os.Getenv("PRINCIPALS"), ","), + RequestedPrincipals: strings.Split(os.Getenv("SSH_PRINCIPALS"), ","), SSHKeyPath: os.Getenv("SSH_KEY_PATH"), SSHKeyPassword: os.Getenv("SSH_KEY_PASSWORD"), SSHSigningServerURL: os.Getenv("SSH_SIGNING_SERVER_URL"), @@ -88,7 +88,12 @@ func parseEnvironment() (*Config, error) { } } - // TODO: Do more config error validation + if config.SSHSigningServerAddress != "" || config.SSHCaAuthorizedKeysPath != "" || config.SSHCaKeyPath != "" { + if config.SSHSigningServerAddress == "" || config.SSHCaAuthorizedKeysPath == "" || config.SSHCaKeyPath == "" { + return nil, fmt.Errorf("SSH_CA_KEY_PATH, SSH_CA_AUTHORIZED_KEYS_PATH, SSH_SIGNING_SERVER_LISTEN_ADDRESS needs to be provided") + } + + } if config.SSHSigningServerURL != "" && len(config.RequestedPrincipals) == 0 { return nil, fmt.Errorf("When SSH_SIGNING_SERVER_URL is set a list of principals needs to be provided") @@ -162,19 +167,25 @@ func setupKeyring(config *Config) (agent.ExtendedAgent, error) { } if config.SSHSigningServerURL != "" { - // TODO: Do something with the errors var errors []error for _, signer := range signers { userCert, err := fetchUserCert(config.SSHSigningServerURL, signer.Signer, config.Command, config.Args, config.RequestedPrincipals) if err != nil { - errors = append(errors, err) + errors = append(errors, fmt.Errorf("fetchUserCert for %s failed: %v", signer.Comment, err)) continue } certificates = append(certificates, sshagent.SSHCertificate{ Certificate: userCert, - Comment: "key " + config.SSHKeyPath, + Comment: "key " + signer.Comment, }) } + if len(certificates) == 0 { + errStr := "" + for _, err := range errors { + errStr += err.Error() + } + return nil, fmt.Errorf("Failed to fetch a user cert:\n" + errStr) + } } return sshagent.NewSSHAlgorithmSignerKeyring(signers, certificates) diff --git a/kms/google/kms-signer.go b/kms/google/kms-signer.go index 1da7f40..3e86a8b 100644 --- a/kms/google/kms-signer.go +++ b/kms/google/kms-signer.go @@ -1,7 +1,5 @@ package google -// TODO: Move to google kms package instead - import ( "context" "crypto" diff --git a/server/main.go b/server/main.go index 1d7874a..1fccc86 100644 --- a/server/main.go +++ b/server/main.go @@ -85,13 +85,28 @@ func (s *SigningServer) VerifyCertificateRequest(certRequest *CertificateRequest if err != nil { return nil, err } - // TODO: Check if challenge expired + now := time.Now().UTC() + + // Check if challenge expired + issueTimeStamp, err := time.Parse(time.RFC3339Nano, value.Timestamp) + if err != nil { + return nil, err + } + if issueTimeStamp.After(now.Add(30 * time.Second)) { + return nil, fmt.Errorf("challenge expired") + } + + // Fetch key from allowed map allowedKey := s.allowedKeysMap[certRequest.PublicKey] if allowedKey == nil { return nil, nil } - // TODO: Check if allowedKey is expired? + + // Disallowed if the key expired + if allowedKey.ExpiresAt.Before(now) { + return nil, fmt.Errorf("key expired") + } payload := GenerateSigningPayload(certRequest) @@ -149,7 +164,7 @@ func (s *SigningServer) GenerateChallenge() (challenge *Challenge, err error) { } jsonBytes, err := json.Marshal(&challengeValue{ - Timestamp: time.Now().UTC().Format("2006-01-02T15:04:05.999Z"), + Timestamp: time.Now().UTC().Format(time.RFC3339Nano), Random: randomBytes, }) diff --git a/server/utils.go b/server/utils.go index 9890ee6..390d3f3 100644 --- a/server/utils.go +++ b/server/utils.go @@ -4,6 +4,7 @@ import ( "crypto/rand" "fmt" "strings" + "time" "golang.org/x/crypto/ssh" ) @@ -22,6 +23,7 @@ func GenerateRamdomBytes(length int) (value []byte, err error) { type AllowedKey struct { Index int Key ssh.PublicKey + ExpiresAt time.Time Comment string Principals []string ValidBefore uint64 @@ -36,6 +38,10 @@ func ParseAuthorizedKeys(lines []string) ([]AllowedKey, error) { // http://man7.org/linux/man-pages/man8/sshd.8.html#AUTHORIZED_KEYS_FILE_FORMAT // https://github.com/openssh/openssh-portable/blob/master/PROTOCOL.certkeys for i, line := range lines { + if strings.HasPrefix(line, "#") { + continue + } + publicKey, comment, options, _, err := ssh.ParseAuthorizedKey([]byte(line)) if err != nil { return nil, fmt.Errorf("failed to parse line '%s': %v", line, err) @@ -44,7 +50,8 @@ func ParseAuthorizedKeys(lines []string) ([]AllowedKey, error) { key := AllowedKey{ Index: i, Key: publicKey, - ValidBefore: ssh.CertTimeInfinity, // TODO: Set a better value // uint64(time.Now().Add(time.Minute * 60).Unix()), + ExpiresAt: time.Unix(1<<63-62135596801, 999999999), // MaxTime + ValidBefore: uint64(time.Now().Add(time.Minute * 60).Unix()), Comment: comment, Principals: []string{}, Options: map[string]string{}, @@ -64,10 +71,16 @@ func ParseAuthorizedKeys(lines []string) ([]AllowedKey, error) { case "agent-forwarding": key.Extensions["permit-agent-forwarding"] = value case "command": - // TODO: Don't allow empty commands + if value == "" { + return nil, fmt.Errorf("empty command not allowed") + } key.Options["force-command"] = value case "expiry-time": - // TODO: Use this to ignore key after date + expiresAt, err := time.Parse("2006010215040599", value) + if err != nil { + return nil, fmt.Errorf("expiry-time not valid format %s", value) + } + key.ExpiresAt = expiresAt case "from": // TODO: Convert wildcard matching to CIDR address/masklen notation key.Options["source-address"] = value