Introduce `aws_rolesanywhere_trustanchor` BundlePublisher plugin #5048

ajay1135 · 2024-04-08T17:03:14Z

Pull Request check list

Commit conforms to CONTRIBUTING.md?
Proper tests/regressions included?
Documentation updated?

Affected functionality

Introduces the aws_rolesanywhere_trustanchor BundlePublisher 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:

Roles Anywhere allows for at most two certificates per trust anchor (if more are provided, I believe only the first two are used (I'm yet to test this, but I will do so))
- The idea behind supporting two certificates is to support seamless certificate rotation, but I'm not sure how limiting this is from a SPIRE POV
Roles Anywhere doesn't support CRL DPs or OCSP endpoints and instead supports a separate CRL resource (revocation is disregarded in this PR)
The order in which trust anchors appear when calling rolesanywhere:ListTrustAnchors isn'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

amartinezfayo

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

pkg/server/plugin/bundlepublisher/awsrolesanywhere/awsrolesanywhere.go

amartinezfayo · 2024-04-18T00:19:29Z

pkg/server/plugin/bundlepublisher/awsrolesanywhere/awsrolesanywhere.go

+ }
+
+ p.setBundle(req.GetBundle())
+ p.log.Debug("Trust anchor bundle updated", "ARN", trustAnchorArn)


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.

Will address in my next revision.

amartinezfayo · 2024-04-18T01:12:52Z

pkg/server/plugin/bundlepublisher/awsrolesanywhere/awsrolesanywhere.go

+ // 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
+ }


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.

Yeah, that makes sense. I will change it so that it works as you suggested in my next revision.

ajay1135 · 2024-04-19T12:33:46Z

Thanks for the feedback! I think what you said makes sense. I'll update the PR when I get a chance.

amartinezfayo · 2024-05-13T13:41:14Z

conf/server/server_full.conf

+ # # 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"


This should be updated to trust_anchor_id.

Thanks for the catch. Will update in the next revision.

amartinezfayo · 2024-05-13T13:43:39Z

pkg/server/plugin/bundlepublisher/awsrolesanywhere/awsrolesanywhere.go

+// PublishBundle puts the bundle in the first Roles Anywhere trust anchor
+// found with the configured name. If one doesn't exist, it is created.


This should be updated to reflect the updated implementation.

Will update in the next revision.

pkg/server/plugin/bundlepublisher/awsrolesanywhere/awsrolesanywhere.go

amartinezfayo

Thank you @ajay1135 for addressing the comments, it looks great!
I only have a few comments related with the tests. After addressing those, we should be able to merge.

pkg/server/plugin/bundlepublisher/awsrolesanywhere/awsrolesanywhere_test.go

doc/plugin_server_bundlepublisher_aws_rolesanywhere_trustanchor.md

* Implement and add tests for the plugin Signed-off-by: Ajay Gupta <apg76@cornell.edu>

…lugin Signed-off-by: Ajay Gupta <apg76@cornell.edu>

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) * Add note about how this plugin is only supported when an UpstreamAuthority plugin is also used * Use ID instead of trust anchor name to identify trust anchors, as it's unique * Check that certificate bundles don't exceed a length of 8000 before making the UpdateTrustAnchor API call * Make corresponding changes to unit tests Signed-off-by: Ajay Gupta <apg76@cornell.edu>

Co-authored-by: Agustín Martínez Fayó <amartinezfayo@gmail.com> Signed-off-by: ajay1135 <32616412+ajay1135@users.noreply.github.com>

ajay1135 · 2024-07-01T01:56:33Z

Thanks for the comments, and sorry for the delay. I've applied your suggestions and addressed your other comment as well.

amartinezfayo

Thank you @ajay1135 for this contribution!

…ffe#5048) * Introduce the aws_rolesanywhere_trustanchor BundlePublisher plugin * Implement and add tests for the plugin Signed-off-by: Ajay Gupta <apg76@cornell.edu> * Add documentation for aws_rolesanywhere_trustanchor BundlePublisher plugin Signed-off-by: Ajay Gupta <apg76@cornell.edu> * Apply suggestions from code review Co-authored-by: Agustín Martínez Fayó <amartinezfayo@gmail.com> Signed-off-by: ajay1135 <32616412+ajay1135@users.noreply.github.com> * Address comments on PR * Only required rolesanywhere:UpdateTrustAnchor permissions (no creating or listing) * Add note about how this plugin is only supported when an UpstreamAuthority plugin is also used * Use ID instead of trust anchor name to identify trust anchors, as it's unique * Check that certificate bundles don't exceed a length of 8000 before making the UpdateTrustAnchor API call * Make corresponding changes to unit tests Signed-off-by: Ajay Gupta <apg76@cornell.edu> * Apply suggestions from code review Co-authored-by: Agustín Martínez Fayó <amartinezfayo@gmail.com> Signed-off-by: ajay1135 <32616412+ajay1135@users.noreply.github.com> --------- Signed-off-by: Ajay Gupta <apg76@cornell.edu> Signed-off-by: ajay1135 <32616412+ajay1135@users.noreply.github.com> Co-authored-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

ajay1135 requested review from evan2645, amartinezfayo, azdagron, MarcosDY and rturner3 as code owners April 8, 2024 17:03

rturner3 assigned amartinezfayo Apr 9, 2024

amartinezfayo reviewed Apr 18, 2024

View reviewed changes

ajay1135 requested a review from amartinezfayo May 6, 2024 02:49

amartinezfayo reviewed May 16, 2024

View reviewed changes

ajay1135 force-pushed the feature/aws-rolesanywhere-bundlepublisher branch from 93b1438 to 7713757 Compare June 16, 2024 01:09

amartinezfayo reviewed Jun 18, 2024

View reviewed changes

pkg/server/plugin/bundlepublisher/awsrolesanywhere/awsrolesanywhere_test.go Outdated Show resolved Hide resolved

pkg/server/plugin/bundlepublisher/awsrolesanywhere/awsrolesanywhere_test.go Show resolved Hide resolved

amartinezfayo reviewed Jun 23, 2024

View reviewed changes

doc/plugin_server_bundlepublisher_aws_rolesanywhere_trustanchor.md Outdated Show resolved Hide resolved

ajay1135 and others added 5 commits July 1, 2024 01:53

Introduce the aws_rolesanywhere_trustanchor BundlePublisher plugin

11f14d1

* Implement and add tests for the plugin Signed-off-by: Ajay Gupta <apg76@cornell.edu>

Add documentation for aws_rolesanywhere_trustanchor BundlePublisher p…

99e18b0

…lugin Signed-off-by: Ajay Gupta <apg76@cornell.edu>

Apply suggestions from code review

82c6064

Co-authored-by: Agustín Martínez Fayó <amartinezfayo@gmail.com> Signed-off-by: ajay1135 <32616412+ajay1135@users.noreply.github.com>

Apply suggestions from code review

153943b

Co-authored-by: Agustín Martínez Fayó <amartinezfayo@gmail.com> Signed-off-by: ajay1135 <32616412+ajay1135@users.noreply.github.com>

ajay1135 force-pushed the feature/aws-rolesanywhere-bundlepublisher branch from 1ffa20c to 153943b Compare July 1, 2024 01:55

amartinezfayo approved these changes Jul 1, 2024

View reviewed changes

amartinezfayo added this to the 1.10.1 milestone Jul 1, 2024

amartinezfayo merged commit be8667e into spiffe:main Jul 1, 2024
33 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `aws_rolesanywhere_trustanchor` BundlePublisher plugin #5048

Introduce `aws_rolesanywhere_trustanchor` BundlePublisher plugin #5048

ajay1135 commented Apr 8, 2024 •

edited

Loading

amartinezfayo left a comment

amartinezfayo Apr 18, 2024

ajay1135 May 6, 2024

amartinezfayo Apr 18, 2024

ajay1135 May 6, 2024

ajay1135 commented Apr 19, 2024

amartinezfayo May 13, 2024

ajay1135 May 19, 2024

amartinezfayo May 13, 2024

ajay1135 May 19, 2024

amartinezfayo left a comment

ajay1135 commented Jul 1, 2024

amartinezfayo left a comment

		// PublishBundle puts the bundle in the first Roles Anywhere trust anchor
		// found with the configured name. If one doesn't exist, it is created.

Introduce aws_rolesanywhere_trustanchor BundlePublisher plugin #5048

Introduce aws_rolesanywhere_trustanchor BundlePublisher plugin #5048

Conversation

ajay1135 commented Apr 8, 2024 • edited Loading

amartinezfayo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajay1135 commented Apr 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amartinezfayo left a comment

Choose a reason for hiding this comment

ajay1135 commented Jul 1, 2024

amartinezfayo left a comment

Choose a reason for hiding this comment

Introduce `aws_rolesanywhere_trustanchor` BundlePublisher plugin #5048

Introduce `aws_rolesanywhere_trustanchor` BundlePublisher plugin #5048

ajay1135 commented Apr 8, 2024 •

edited

Loading