Introduce aws_rolesanywhere_trustanchor BundlePublisher plugin
#5048
Conversation
* Implement and add tests for the plugin Signed-off-by: Ajay Gupta <apg76@cornell.edu>
…lugin Signed-off-by: Ajay Gupta <apg76@cornell.edu>
ajay1135
requested review from
evan2645,
amartinezfayo,
azdagron,
MarcosDY and
rturner3
as code owners
There was a problem hiding this comment.
Thank you @ajay1135 for this contribution!
My main concern is related with having the plugin configured with a trust anchor name, rather than a specific trust anchor. I see a lot of disadvantages with this approach, the main one being that's prone to configuration errors. This is exacerbated by the fact that there may be multiple trust anchors with the same name, so you can't uniquely identify a trust anchor by its name. Peeking the first of the list returned with the matching name doesn't seem to be a robust solution for this. Updating the wrong trust anchor and have the user trusting an incorrect trust anchor can be pretty bad.
My preference is to configure the plugin with a specific trust anchor ID.
I left some feedback.
doc/plugin_server_bundlepublisher_aws_rolesanywhere_trustanchor.md
Outdated
Show resolved
Hide resolved
pkg/server/plugin/bundlepublisher/awsrolesanywhere/awsrolesanywhere.go
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| p.setBundle(req.GetBundle()) | ||
| p.log.Debug("Trust anchor bundle updated", "ARN", trustAnchorArn) |
There was a problem hiding this comment.
I would have this message logged as "Bundle published" rather than "Trust anchor bundle updated". This is logged with a plugin_name field, which in this case will be "aws_rolesanywhere_trustanchor", telling that's a trust anchor what has been updated.
We use snake case for the field names, so "ARN" should be "arn".
I would also include the trust anchor name, using a "trust_anchor_name" field.
There was a problem hiding this comment.
Will address in my next revision.
| // Check whether there already exists a trust anchor with the name requested | ||
| // If so, perform an update of its trust bundle | ||
| var trustAnchor rolesanywheretypes.TrustAnchorDetail | ||
| foundTrustAnchor := false | ||
| prevNextToken := "" | ||
| for ok := true; ok; { | ||
| // List trust anchors | ||
| listTrustAnchorsInput := rolesanywhere.ListTrustAnchorsInput{} | ||
| if prevNextToken != "" { | ||
| listTrustAnchorsInput.NextToken = &prevNextToken | ||
| } | ||
| listTrustAnchorsOutput, err := p.rolesAnywhereClient.ListTrustAnchors(ctx, &listTrustAnchorsInput) | ||
| if err != nil { | ||
| return nil, status.Errorf(codes.Internal, "failed to list trust anchors: %v", err) | ||
| } | ||
|
|
||
| // Iterate through trust anchors in response | ||
| for _, curTrustAnchor := range listTrustAnchorsOutput.TrustAnchors { | ||
| if *curTrustAnchor.Name == config.TrustAnchorName { | ||
| trustAnchor = curTrustAnchor | ||
| foundTrustAnchor = true | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if foundTrustAnchor { | ||
| break | ||
| } | ||
|
|
||
| if listTrustAnchorsOutput.NextToken == nil { | ||
| break | ||
| } | ||
| prevNextToken = *listTrustAnchorsOutput.NextToken | ||
| } | ||
|
|
||
| trustAnchorArn := "" | ||
| if foundTrustAnchor { | ||
| // Update the trust anchor that was found | ||
| updateTrustAnchorInput := rolesanywhere.UpdateTrustAnchorInput{ | ||
| TrustAnchorId: trustAnchor.TrustAnchorId, | ||
| Source: &rolesanywheretypes.Source{ | ||
| SourceType: rolesanywheretypes.TrustAnchorTypeCertificateBundle, | ||
| SourceData: &rolesanywheretypes.SourceDataMemberX509CertificateData{ | ||
| Value: bundleStr, | ||
| }, | ||
| }, | ||
| } | ||
| updateTrustAnchorOutput, err := p.rolesAnywhereClient.UpdateTrustAnchor(ctx, &updateTrustAnchorInput) | ||
| if err != nil { | ||
| return nil, status.Errorf(codes.Internal, "failed to update trust anchor: %v", err) | ||
| } | ||
| trustAnchorArn = *updateTrustAnchorOutput.TrustAnchor.TrustAnchorArn | ||
| } else { | ||
| // Create a new trust anchor, since an existing one with the requsted name couldn't be found | ||
| createTrustAnchorInput := rolesanywhere.CreateTrustAnchorInput{ | ||
| Name: &config.TrustAnchorName, | ||
| Source: &rolesanywheretypes.Source{ | ||
| SourceType: rolesanywheretypes.TrustAnchorTypeCertificateBundle, | ||
| SourceData: &rolesanywheretypes.SourceDataMemberX509CertificateData{ | ||
| Value: bundleStr, | ||
| }, | ||
| }, | ||
| Enabled: func() *bool { b := true; return &b }(), | ||
| } | ||
|
|
||
| createTrustAnchorOutput, err := p.rolesAnywhereClient.CreateTrustAnchor(ctx, &createTrustAnchorInput) | ||
| if err != nil { | ||
| return nil, status.Errorf(codes.Internal, "failed to create trust anchor: %v", err) | ||
| } | ||
| trustAnchorArn = *createTrustAnchorOutput.TrustAnchor.TrustAnchorArn | ||
| } |
There was a problem hiding this comment.
Giving the responsibility to create the trust anchor to the plugin has a lot of disadvantages in my opinion.
It makes the publishing logic a lot more complex than what would be needed if it assumes that the trust anchor already exists, but more importantly, it's prone to configuration errors that can lead to create a trust anchor with a wrong name, and not update the proper trust anchor. The fact that there could be multiple trust anchors with the same name doesn't help either, and contributes to make this more error prone.
It also requires to grant additional permissions, when we are only interested in keeping a trust anchor updated.
I think that's better to give the responsibility to create the trust anchor to the user, and just configure the plugin with a specific trust anchor to update. Only the required permissions to update a trust anchor would be required in this case.
There was a problem hiding this comment.
Yeah, that makes sense. I will change it so that it works as you suggested in my next revision.
|
Thanks for the feedback! I think what you said makes sense. I'll update the PR when I get a chance. |
Co-authored-by: Agustín Martínez Fayó <amartinezfayo@gmail.com> Signed-off-by: ajay1135 <32616412+ajay1135@users.noreply.github.com>
* Only required rolesanywhere:UpdateTrustAnchor permissions (no creating or listing) * Use ID instead of trust anchor name to identify trust anchors, as it's unique * Make corresponding changes to unit tests Signed-off-by: Ajay Gupta <apg76@cornell.edu>
| # # secret_access_key = "" | ||
|
|
||
| # # trust_anchor_name: The AWS IAM Roles Anywhere trust anchor name to which to store the trust bundle. Default: "". | ||
| # # trust_anchor_name = "spire-trust-anchor" |
There was a problem hiding this comment.
This should be updated to trust_anchor_id.
There was a problem hiding this comment.
Thanks for the catch. Will update in the next revision.
| // PublishBundle puts the bundle in the first Roles Anywhere trust anchor | ||
| // found with the configured name. If one doesn't exist, it is created. |
There was a problem hiding this comment.
This should be updated to reflect the updated implementation.
There was a problem hiding this comment.
Will update in the next revision.
| bundleStr := string(bundleBytes) | ||
|
|
||
| // Update the trust anchor that was found | ||
| updateTrustAnchorInput := rolesanywhere.UpdateTrustAnchorInput{ |
There was a problem hiding this comment.
The maximum length of the SourceData is 8000. Do you think that it would be problematic in your use case?
That would be around 11 certs in the bundle when no upstream authority is used. I think that we should have a validation, and if it's larger than 8000, fail with a descriptive error before trying to update. Otherwise, the error response from the AWS API will include the SourceData, which can flood the logs.
There was a problem hiding this comment.
Good catch, I can add that check in. But I think what will usually be more limiting is the fact that the maximum bundle size that can be used for a trust anchor is two currently (see the "Certificates per trust anchor" limit), and it isn't adjustable either. For example, when I try to update a trust anchor with a bundle of size six, I get the following error:
An error occurred (ValidationException) when calling the UpdateTrustAnchor operation: Unable to update a TrustAnchor with a certificate bundle of size 6.
At least the error response doesn't contain the bundle contents, so maybe the concern about flooding the logs isn't as relevant here, but it's still an error nonetheless. I think the intent behind this limit being two is to support CA certificate rotation, without loss of availability. So in general, the service is expecting a single CA certificate to be uploaded as a trust anchor (and two when the CA certificate needs to be rotated).
Given this, in what scenarios will the trust bundle contain more than a single certificate? And in those cases, would it be okay to have this plugin publish just one of those certificates to the Roles Anywhere trust anchor? I'm guessing this behavior would make the plugin inconsistent with the functionality of the other bundle publisher plugins though - I'm not sure if that's okay. If we do go with this approach and additional certificates are needed, they can be provided in the request to CreateSession (the Roles Anywhere credentialing API), in the X-Amz-X509-Chain HTTP header (I have to test whether there's a limit here, but it should definitely be greater than one).
What are your thoughts on this?
There was a problem hiding this comment.
This is only a problem in deployments that don't use an UpstreamAuthority plugin.
For those deployments that don't use an UpstreamAuthority plugin, SPIRE will be adding a new X509 certificate to the bundle when the current active X509 authority has passed 5/6 of its lifetime.
What really matters here is the ability (or lack of) publishing the certificates in the bundle that are still valid. And that could be at most two per server. The problem with this is that in HA deployments, each server will be adding its own certificates to the bundle because the bundle is formed by all the authorities of all the servers.
Considering that, it doesn't seem that this plugin would work in HA deployments that don't have an UpstreamAuthority plugin, which reduces the chances of being used in production environments when there is no upstream authority.
Having those considerations, I'm leaning towards recommending that this plugin should only be supported when an UpstreamAuthority plugin is used. Would that be a problem in your use case?
There was a problem hiding this comment.
I think that should work for now, thanks.
How do you suggest I check that a plugin of a different type has also been configured? Is there any precedence for it? I could potentially "fake" it by adding another configuration field for this plugin, which will allow the user to acknowledge (so a boolean field) that they are also using an UpstreamAuthority plugin. It could also be a more general acknowledgement field, so that the user will only configure the plugin if they know for sure they want to use it despite the limitations.
There was a problem hiding this comment.
Unfortunately, there is no easy way for a plugin to check if other plugin has been configured. Having a setting to explicitly tell that there is an UpstreamAuthority plugin may work, but at the same time it feels a little confusing to me. I would suggest that we start by having a very clear documentation about this plugin being only supported when an UpstreamAuthority plugin is used.
There was a problem hiding this comment.
Hey @ajay1135 I just wanted to provide a quick summary of what I think is pending:
- Document that this plugin is only supported when an UpstreamAuthority plugin is used. Probably with a bold note at the very beginning of the .md document.
- Check that the bundle has 1) no more than 2 certs before is being published and 2) doesn't exceed the maximum length of 8000.
- Have 2) covered by unit tests.
Pull Request check list
Affected functionality
Introduces the
aws_rolesanywhere_trustanchorBundlePublisher plugin. When the server's trust bundle is updated, it will keep the corresponding AWS IAM Roles Anywhere trust anchor updated as well. Users that want to use IAM Roles Anywhere to access AWS using temporary credentials from outside of AWS (on-prem, another Cloud Service Provider, etc.) can consider using this plugin.Description of change
This plugin needs to be configured with a
trust_anchor_name, which contains the name of the trust anchor to create or update. If there are multiple with the same name, the first that is found when listing trust anchors (rolesanywhere:ListTrustAnchors) is used. And if one can't be found, a new trust anchor is created.A few things to be aware of:
rolesanywhere:ListTrustAnchorsisn't an "inserted first" order (instead it's lexicographical based on UUID (not name) of the trust anchor)Which issue this PR fixes
Not sure if it completely fixes it, but this PR does address part of #4963