Skip to content

builder: check Org/OU ARNs during prepare #494

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.

Sign up for GitHub

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

Open
lbajolet-hashicorp wants to merge 1 commit into
base: main
Choose a base branch
from
Open

builder: check Org/OU ARNs during prepare #494

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions builder/common/ami_config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,12 @@ import (
"github.com/hashicorp/packer-plugin-sdk/template/interpolate"
)

// OrgARNRegexp validates organisation ARNs
var OrgARNRegexp = regexp.MustCompile(`^arn:aws:organizations::\d{12}:organization\/o-[a-z0-9]{10,32}`)

// ARNRegexp validates organisation unit (OU) ARNs
var OUARNRegexp = regexp.MustCompile(`^arn:aws:organizations::\d{12}:ou\/o-[a-z0-9]{10,32}\/ou-[0-9a-z]{4,32}-[0-9a-z]{8,32}`)

// AMIConfig is for common configuration related to creating AMIs.
type AMIConfig struct {
// The name of the resulting AMI that will appear when managing AMIs in the
Expand Down Expand Up @@ -195,6 +201,34 @@ func (c *AMIConfig) Prepare(accessConfig *AccessConfig, ctx *interpolate.Context

errs = append(errs, c.prepareRegions(accessConfig)...)

for _, orgARN := range c.AMIOrgArns {
if !OrgARNRegexp.MatchString(orgARN) {
if OUARNRegexp.MatchString(orgARN) {
errs = append(errs,
fmt.Errorf("Organisation ARN %q looks like a OU ARN, "+
"it should be part of the `ami_ou_arns` collection instead", orgARN))
continue
}

errs = append(errs, fmt.Errorf("Invalid Organisation ARN %q, expect something that looks like the "+
"following: arn:aws:organizations::123456789012:organization/o-00000aaaaa", orgARN))
}
}

for _, ouARN := range c.AMIOuArns {
if !OUARNRegexp.MatchString(ouARN) {
if OrgARNRegexp.MatchString(ouARN) {
errs = append(errs,
fmt.Errorf("Organisation Unit ARN %q looks like a Organisation ARN, "+
"it should be part of the `ami_org_arns` collection instead", ouARN))
continue
}

errs = append(errs, fmt.Errorf("Invalid Organisation Unit ARN %q, expect something that looks like the "+
"following: arn:aws:organizations::123456789012:ou/o-12345abcdef/ou-5678a-0000aaaa", ouARN))
}
}

// Prevent sharing of default KMS key encrypted volumes with other aws users
if len(c.AMIUsers) > 0 || len(c.AMIOrgArns) > 0 || len(c.AMIOuArns) > 0 {
if len(c.AMIKmsKeyId) == 0 && len(c.AMIRegionKMSKeyIDs) == 0 && c.AMIEncryptBootVolume.True() {
Expand Down
75 changes: 75 additions & 0 deletions builder/common/ami_config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,3 +283,78 @@ func TestAMINameValidation(t *testing.T) {
}

}

// Make sure only valid ARNs are accepted
func TestAMIARNValidation(t *testing.T) {
tests := []struct {
name string
OrgARN []string
OUARN []string
expectErr bool
Copy link
Contributor

@JenGoldstrich JenGoldstrich Jul 17, 2024

Choose a reason for hiding this comment

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

I would consider extending this to check the specific error returned, since there's several different cases that could change and these tests wouldn't fail

Copy link
Contributor Author

@lbajolet-hashicorp lbajolet-hashicorp Jul 17, 2024

Choose a reason for hiding this comment

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

I'm less sure about this, I think checking if the configs provoke an error when expected is more important than the contents of those errors, so I'm tempted to leave it as-is, but please feel free to push back if you think it's important

}{
{
"Nothing defined, should succeed",
nil,
nil,
false,
},
{
"Invalid OrgARN defined, should fail",
[]string{"arn:aws:organizations::1234"},
nil,
true,
},
{
"Invalid OUARN defined, should fail",
nil,
[]string{"arn:aws:organizations::1234:ou/o-1234567890/ou-aced"},
true,
},
{
"Valid OrgARN, invalid OUARN defined, should fail",
[]string{"arn:aws:organizations::123456789012:organization/o-1234567890000"},
[]string{"arn:aws:organizations::1234:ou/o-1234567890/ou-aced"},
true,
},
{
"Invalid OrgARN, valid OUARN defined, should fail",
[]string{"arn:aws:organizations::1234:organization"},
[]string{"arn:aws:organizations::123456789012:ou/o-12345abcdef/ou-ab12-12345678"},
true,
},
{
"Valid OrgARN, valid OUARN defined, should succeed",
[]string{"arn:aws:organizations::123456789012:organization/o-1234567890000"},
[]string{"arn:aws:organizations::123456789012:ou/o-12345abcdef/ou-ab12-12345678"},
false,
},
{
"Valid OrgARN as OU ARN, should fail",
[]string{"arn:aws:organizations::123456789012:ou/o-12345abcdef/ou-ab12-12345678"},
nil,
true,
},
{
"Valid OU ARN as Org ARN, should fail",
nil,
[]string{"arn:aws:organizations::123456789012:organization/o-1234567890000"},
true,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
c := testAMIConfig()
c.AMIOrgArns = tt.OrgARN
c.AMIOuArns = tt.OUARN

errs := c.Prepare(FakeAccessConfig(), nil)
if len(errs) != 0 && !tt.expectErr {
t.Errorf("Unexpected error %v; expected none.", errs)
}
if len(errs) == 0 && tt.expectErr {
t.Errorf("Expected error, got none.")
}
})
}
}