Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add logging during awskms auto-unseal #9794

Merged
merged 7 commits into from
Sep 28, 2020
Merged

Conversation

tvoran
Copy link
Member

@tvoran tvoran commented Aug 21, 2020

Adds debug and warn logging around AWS credential chain generation, specifically to help users understand why auto-unseal isn't working on AWS, by logging which role is being used in the case of a webidentity token.

Also in the case of webidentity, this tests retrieving credentials, and logs a warning if it has trouble, in which case it falls through to using creds from the instance metadata, which may still fail, but with a warning logged at least users can tell why the webidentity auth failed.

Depends on hashicorp/go-kms-wrapping#22

Example logging with level=trace when auto-unseal fails:

before:


Error parsing Seal configuration: error fetching AWS KMS wrapping key information: AccessDeniedException: User: arn:aws:sts::000000000000:assumed-role/eksctl-tvoran-eks-dev-nodegroup-s-NodeInstanceRole-000000000000/i-0000000000000 is not authorized to perform: kms:DescribeKey on resource: arn:aws:kms:us-west-2:000000000000:key/00000000-00000000-00000000-00000000
    status code: 400, request id: 00000000-00000000-00000000-00000000
stream closed

after:


2020-08-20T20:33:47.809Z [INFO]  proxy environment: http_proxy= https_proxy= no_proxy=
2020-08-20T20:33:47.812Z [DEBUG] service_registration.kubernetes: "namespace": "default"
2020-08-20T20:33:47.812Z [DEBUG] service_registration.kubernetes: "pod_name": "vault-0"
2020-08-20T20:33:47.812Z [DEBUG] seal.awskms: adding web identity provider: roleARN=arn:aws:iam::000000000000:role/eksctl-my-cluster-addon-iamserviceaccoun-Role1-0000000000000
2020-08-20T20:33:48.798Z [WARN]  seal.awskms: error assuming role: roleARN=arn:aws:iam::000000000000:role/eksctl-my-cluster-addon-iamserviceaccoun-Role1-0000000000000 tokenPath=/var/run/secrets/eks.amazonaws.com/serviceaccount/token sessionName= err="WebIdentityErr: failed to retrieve credentials
caused by: AccessDenied: Not authorized to perform sts:AssumeRoleWithWebIdentity
    status code: 403, request id: 00000000-00000000-00000000-00000000"
Error parsing Seal configuration: error fetching AWS KMS wrapping key information: AccessDeniedException: User: arn:aws:sts::000000000000:assumed-role/eksctl-tvoran-eks-dev-nodegroup-s-NodeInstanceRole-0000000000000/i-0000000000000000 is not authorized to perform: kms:DescribeKey on resource: arn:aws:kms:us-west-2:000000000000:key/00000000-00000000-00000000-00000000
    status code: 400, request id: 00000000-00000000-00000000-00000000
stream closed

Adds debug and warn logging around AWS credential chain generation,
specifically to help users debugging auto-unseal problems on AWS, by
logging which role is being used in the case of a webidentity token.

Adds a deferred call to flush the log output as well, to ensure logs
are output in the event of an initialization failure.
@tvoran tvoran requested a review from a team August 21, 2020 00:54
@@ -51,6 +55,8 @@ func (c *CredentialsConfig) GenerateCredentialChain() (*credentials.Credentials,
SecretAccessKey: c.SecretKey,
SessionToken: c.SessionToken,
}})
c.Logger.Debug("added static credential provider", "AccessKey", c.AccessKey)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

c.Logger could potentially be nil if none is passed into CredentialsConfig so these calls would panic. Let's wrap logger calls with a nil check.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, added in 98adda6

@@ -98,7 +98,7 @@ func (h *CLIHandler) Auth(c *api.Client, m map[string]string) (*api.Secret, erro
headerValue = ""
}

creds, err := RetrieveCreds(m["aws_access_key_id"], m["aws_secret_access_key"], m["aws_security_token"])
creds, err := RetrieveCreds(m["aws_access_key_id"], m["aws_secret_access_key"], m["aws_security_token"], hclog.Default())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not derive any of the logging properties from the main command, so it might result in different format, log level, etc.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would we get the logging properties from the main command here? I haven't found a way to do that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The log would be from the client (result from a vault login command) for this particular case so it might be fine, though hclog.Default() logs up to Info only, so it would skip any Debug output.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not seeing anywhere else in the vault login command that verbosity levels are set, so in 2271d6c I added a log_level option to the aws cli login command, and used that to configure the logger passed to RetrieveCreds(). Or maybe it's preferable to just pass a null logger here?

@calvn calvn added this to the 1.6 milestone Sep 28, 2020
go.mod Outdated
@@ -58,7 +58,7 @@ require (
github.com/hashicorp/go-cleanhttp v0.5.1
github.com/hashicorp/go-gcp-common v0.6.0
github.com/hashicorp/go-hclog v0.14.1
github.com/hashicorp/go-kms-wrapping v0.5.12
github.com/hashicorp/go-kms-wrapping v0.5.15
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be using v0.5.16 right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, just fixed in e4f2e2b

@tvoran tvoran merged commit 10c0ada into master Sep 28, 2020
@tvoran tvoran deleted the VAULT-92/awskms-logging branch September 28, 2020 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants