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

Organization List Feature in Server AWS Node Attester Plugin "aws_iid" #4838

Merged
merged 85 commits into from
Apr 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
85 commits
Select commit Hold shift + click to select a range
a3c1723
Add New Organization Feature
rushi47 Jan 26, 2024
bb0b0b8
support for retries
rushi47 Mar 8, 2024
4daf960
reset retires
rushi47 Mar 8, 2024
babd6d7
Simply description
rushi47 Mar 18, 2024
d78195a
refactor docs
rushi47 Mar 18, 2024
2b60032
refactor doc
rushi47 Mar 18, 2024
77a1c96
move function for code structure, refactor variable names
rushi47 Mar 18, 2024
d0138f9
remove region check
rushi47 Mar 19, 2024
5f7474c
add optinal param
rushi47 Mar 19, 2024
d00e3e4
remove requirement of region, set default
rushi47 Mar 19, 2024
6061cd6
doc
rushi47 Mar 19, 2024
0b9c761
refactor struct name to reflect orgvalidation
rushi47 Mar 19, 2024
74ab21d
refactor naming to match the style
rushi47 Mar 19, 2024
4ea7ced
indirection to simplify local cache lookups
rushi47 Mar 19, 2024
53cf266
Update doc/plugin_server_nodeattestor_aws_iid.md
rushi47 Apr 8, 2024
6e3a796
Update doc/plugin_server_nodeattestor_aws_iid.md
rushi47 Apr 8, 2024
c46bc63
Update doc/plugin_server_nodeattestor_aws_iid.md
rushi47 Apr 8, 2024
d86447b
code formatting
rushi47 Apr 8, 2024
bdd93df
update documentation to reflect new feature
rushi47 Apr 8, 2024
b058b6f
indent for table
rushi47 Apr 8, 2024
63d0414
refactor
rushi47 Apr 8, 2024
aa9bf03
formatting
rushi47 Apr 8, 2024
ecbc583
refactor error messages
rushi47 Apr 9, 2024
a733311
refactor validation for ttl
rushi47 Apr 9, 2024
d6ac953
update error msg
rushi47 Apr 9, 2024
a75443f
refactor test case for error checking
rushi47 Apr 9, 2024
5574983
test case error msg and extra line
rushi47 Apr 9, 2024
477257e
use interface to prevent memory reservation
rushi47 Apr 9, 2024
116e069
as we read it from config instead of internal it should be invalid arg
rushi47 Apr 9, 2024
4bb278e
function comment and camel casing
rushi47 Apr 9, 2024
dd6e6d1
refactor logic for stale check and code structure
rushi47 Apr 9, 2024
425e030
refactor variable conventions
rushi47 Apr 9, 2024
c92438a
Revert "refactor logic for stale check and code structure"
rushi47 Apr 9, 2024
7de7241
refactor code logic
rushi47 Apr 9, 2024
7e2d944
function comment
rushi47 Apr 9, 2024
d60dd1b
camel casing
rushi47 Apr 9, 2024
acf8437
refactor ttl check, moved to function
rushi47 Apr 9, 2024
09259dd
test for checking ttl expiry
rushi47 Apr 9, 2024
b1e6aab
move function to seprate file
rushi47 Apr 9, 2024
6395767
added tests for organization
rushi47 Apr 10, 2024
dcc4c1f
formatting
rushi47 Apr 10, 2024
8da601d
formatting
rushi47 Apr 10, 2024
33fa8e1
remove extra line
rushi47 Apr 10, 2024
5cc3756
remove extra line
rushi47 Apr 10, 2024
a87ab20
remove extra line
rushi47 Apr 10, 2024
34e2b8d
Merge branch 'main' into aws_node_attestation_org_check_feature
rushi47 Apr 11, 2024
dacdbb7
add org package dependecy
rushi47 Apr 11, 2024
1963e01
linter fixes
rushi47 Apr 12, 2024
168bc17
linter
rushi47 Apr 12, 2024
7713e13
linter
rushi47 Apr 12, 2024
5d0e5c2
linter
rushi47 Apr 12, 2024
c239048
Merge branch 'main' into aws_node_attestation_org_check_feature
rushi47 Apr 12, 2024
d1ed524
update to new version of golang
rushi47 Apr 12, 2024
bb27241
resolve conflicts and update the repo
rushi47 Apr 15, 2024
dcce1c0
linter
rushi47 Apr 15, 2024
2007792
Merge branch 'main' into aws_node_attestation_org_check_feature
MarcosDY Apr 16, 2024
66e23e0
Apply suggestions from code review
rushi47 Apr 16, 2024
570d578
Merge branch 'main' into aws_node_attestation_org_check_feature
rushi47 Apr 16, 2024
e7905eb
we dont require block
rushi47 Apr 16, 2024
4760cd7
remove context when not required
rushi47 Apr 16, 2024
6bcd5fc
seprate context for validate org, and other features
rushi47 Apr 16, 2024
1f36e11
formatting, lints
rushi47 Apr 16, 2024
0d8614c
formatting, lints
rushi47 Apr 16, 2024
0671aed
enrich docs
rushi47 Apr 16, 2024
8e815f1
add support for region
rushi47 Apr 16, 2024
b840804
make organization region configurable
rushi47 Apr 16, 2024
e6a5423
make organization region configurable
rushi47 Apr 16, 2024
328b0b4
typo
rushi47 Apr 16, 2024
44a3216
camel casing
rushi47 Apr 16, 2024
f3339c5
refactor logic
rushi47 Apr 16, 2024
e592089
logging to debug failure in cache refresh
rushi47 Apr 16, 2024
22e2b66
replace boolean as we are using any, better mem management
rushi47 Apr 16, 2024
2e2aa01
refactor logic of validity
rushi47 Apr 16, 2024
98941de
refactor variable name for consistency
rushi47 Apr 16, 2024
28614ff
function name doc
rushi47 Apr 16, 2024
d65480e
add mock clock functionality
rushi47 Apr 17, 2024
391e89e
Merge branch 'main' into aws_node_attestation_org_check_feature
rushi47 Apr 17, 2024
dca217b
refactor tests
rushi47 Apr 17, 2024
cbfdcc0
more tests, for retries and reset
rushi47 Apr 17, 2024
f8c4fc9
add tests case for pagination
rushi47 Apr 17, 2024
b9f5518
update comment
rushi47 Apr 17, 2024
20afd4d
Merge branch 'main' into aws_node_attestation_org_check_feature
rushi47 Apr 17, 2024
799c084
Merge branch 'main' into aws_node_attestation_org_check_feature
MarcosDY Apr 17, 2024
fcbd790
Merge branch 'main' into aws_node_attestation_org_check_feature
MarcosDY Apr 18, 2024
c3b97f7
Merge branch 'main' into aws_node_attestation_org_check_feature
rushi47 Apr 18, 2024
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
23 changes: 23 additions & 0 deletions conf/server/server_full.conf
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,29 @@ plugins {
# # make sure that the underlying root volume has not been detached
# # prior to attestation. Default: false
# # skip_block_device = false

# # verify_organization: Verify if the attesting node's account ID is part of the configured AWS Organization.
# # Make sure that the IAM role formed from the configuration below (e.g., "arn:aws:iam::management_account_id:role/assume_org_role")
# # can be assumed by the spire-server.
# verify_organization {
# # management_account_id: Management account ID, also known as the root account,
# # value will be the respective organization's management/root account ID. It's a required parameter.
# # management_account_id = ""

# # management_account_region: Management account region, specifies the region
# # in which the management account is hosted. It's an optional parameter.
# # Default: us-west-2
# # management_account_region = ""

# # assume_org_role: Assume org role, specifies the role name present in the management
# # account. It's a required parameter.
# # assume_org_role = ""

# # org_account_map_ttl: Org account map TTL, specifies the interval for retrieving the list of accounts present in the Organization.
# # It's an optional parameter. If specified, it should be greater than or equal to the duration of 1m (minute).
# # Default: 3m.
# # org_account_map_ttl = ""
# }
# }
# }

Expand Down
55 changes: 54 additions & 1 deletion doc/plugin_server_nodeattestor_aws_iid.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,9 @@ this plugin resolves the agent's AWS IID-based SPIFFE ID into a set of selectors
| `disable_instance_profile_selectors` | Disables retrieving the attesting instance profile information that is used in the selectors. Useful in cases where the server cannot reach iam.amazonaws.com | false |
| `assume_role` | The role to assume | Empty string, Optional parameter. |
| `partition` | The AWS partition SPIRE server is running in <aws|aws-cn|aws-us-gov> | aws |
| `verify_organization` | Verify that nodes belong to a specified AWS Organization [see below](#enabling-aws-node-attestation-organization-validation) | |

A sample configuration:
Sample configuration:

```hcl
NodeAttestor "aws_iid" {
Expand All @@ -45,6 +46,58 @@ In the following configuration,

assuming AWS IID document sent from the spire agent contains `accountId : 12345678`, the spire server will assume "arn:aws:iam::12345678:role/spire-server-delegate" role before making any AWS call for the node attestation. If `assume_role` is configured, the spire server will always assume the role even if the both the spire-server and the spire agent is deployed in the same account.

## Enabling AWS Node Attestation Organization Validation

For configuring AWS Node attestation method with organization validation following configuration can be used:

| Field Name | Description | Constraints |
|----------------------------|-----------------------------------------------------------------------------------------------|--------------------------------------------|
| management_account_id | Account id of the organzation | required |
| management_account_region | Region of management account id | optional |
| assume_org_role | IAM Role name, with capablities to list accounts | required |
| org_account_map_ttl | Cache the list of accounts for particular time. Should be >= 1 minute. Defaults to 3 minute. | optional |

Using the block `verify_organization` the org validation node attestation method will be enabled. With above configuration spire server will form and try to assume the role as: `arn:aws:iam::management_account_id:role/assume_org_role`. When not used, block ex. `verify_organization = {}` should not be empty, it should be completely removed as its optional or should have all required parameters namely `management_account_id`, `assume_org_role`.

The role under: `assume_role` must be created in the management account: `management_account_id`, and it should have a trust relationship with the role assumed by spire server. Below is a sample policy depicting the permissions required along with the trust relationship that needs to be created in management account.

Policy :

```json
{
"Version": "2012-10-17",
"Statement": [
{
"Action": "organizations:ListAccounts",
"Effect": "Allow",
"Resource": "*",
"Sid": "SpireOrganizationListAccountRole"
}
]
}
```

Trust Relationship

```json
{
"Version": "2012-10-17",
"Statement": [
{
"Sid": "CrossAccountAssumeRolePolicy",
"Effect": "Allow",
"Principal": {
"AWS": [
"arn:aws:iam::account-id-where-spire-is-running:role/spire-control-plane-root-server",
"arn:aws:iam::account-id-where-spire-is-running:role/spire-control-plane-regional-server"
]
},
"Action": "sts:AssumeRole"
}
]
}
```

## Disabling Instance Profile Selectors

In cases where spire-server is running in a location with no public internet access available, setting `disable_instance_profile_selectors = true` will prevent the server from making requests to `iam.amazonaws.com`. This is needed as spire-server will fail to attest nodes as it cannot retrieve the metadata information.
Expand Down
5 changes: 3 additions & 2 deletions go.mod
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
module github.com/spiffe/spire

go 1.22
go 1.22.2

require (
cloud.google.com/go/iam v1.1.7
Expand All @@ -26,6 +26,7 @@ require (
github.com/aws/aws-sdk-go-v2/service/ec2 v1.157.0
github.com/aws/aws-sdk-go-v2/service/iam v1.32.0
github.com/aws/aws-sdk-go-v2/service/kms v1.31.0
github.com/aws/aws-sdk-go-v2/service/organizations v1.27.3
github.com/aws/aws-sdk-go-v2/service/s3 v1.53.0
github.com/aws/aws-sdk-go-v2/service/secretsmanager v1.28.1
github.com/aws/aws-sdk-go-v2/service/sts v1.28.5
Expand Down Expand Up @@ -306,7 +307,7 @@ require (
github.com/tklauser/go-sysconf v0.3.12 // indirect
github.com/tklauser/numcpus v0.6.1 // indirect
github.com/transparency-dev/merkle v0.0.2 // indirect
github.com/twmb/murmur3 v1.1.6 // indirect
github.com/twmb/murmur3 v1.1.8 // indirect
github.com/vbatts/tar-split v0.11.5 // indirect
github.com/xanzy/go-gitlab v0.102.0 // indirect
github.com/xeipuuv/gojsonpointer v0.0.0-20190905194746-02993c407bfb // indirect
Expand Down
6 changes: 4 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -601,6 +601,8 @@ github.com/aws/aws-sdk-go-v2/service/internal/s3shared v1.17.4 h1:uDj2K47EM1reAY
github.com/aws/aws-sdk-go-v2/service/internal/s3shared v1.17.4/go.mod h1:XKCODf4RKHppc96c2EZBGV/oCUC7OClxAo2MEyg4pIk=
github.com/aws/aws-sdk-go-v2/service/kms v1.31.0 h1:yl7wcqbisxPzknJVfWTLnK83McUvXba+pz2+tPbIUmQ=
github.com/aws/aws-sdk-go-v2/service/kms v1.31.0/go.mod h1:2snWQJQUKsbN66vAawJuOGX7dr37pfOq9hb0tZDGIqQ=
github.com/aws/aws-sdk-go-v2/service/organizations v1.27.3 h1:CnPWlONzFX9/yO6IGuKg9sWUE8WhKztYRFbhmOHXjJI=
github.com/aws/aws-sdk-go-v2/service/organizations v1.27.3/go.mod h1:hUHSXe9HFEmLfHrXndAX5e69rv0nBsg22VuNQYl0JLM=
github.com/aws/aws-sdk-go-v2/service/s3 v1.53.0 h1:r3o2YsgW9zRcIP3Q0WCmttFVhTuugeKIvT5z9xDspc0=
github.com/aws/aws-sdk-go-v2/service/s3 v1.53.0/go.mod h1:w2E4f8PUfNtyjfL6Iu+mWI96FGttE03z3UdNcUEC4tA=
github.com/aws/aws-sdk-go-v2/service/secretsmanager v1.28.1 h1:DtKw4TxZT3VrzYupXQJPBqT9ImyobZZE+JIQPPAVxqs=
Expand Down Expand Up @@ -1455,8 +1457,8 @@ github.com/transparency-dev/merkle v0.0.2 h1:Q9nBoQcZcgPamMkGn7ghV8XiTZ/kRxn1yCG
github.com/transparency-dev/merkle v0.0.2/go.mod h1:pqSy+OXefQ1EDUVmAJ8MUhHB9TXGuzVAT58PqBoHz1A=
github.com/tv42/httpunix v0.0.0-20150427012821-b75d8614f926/go.mod h1:9ESjWnEqriFuLhtthL60Sar/7RFoluCcXsuvEwTV5KM=
github.com/twmb/murmur3 v1.1.5/go.mod h1:Qq/R7NUyOfr65zD+6Q5IHKsJLwP7exErjN6lyyq3OSQ=
github.com/twmb/murmur3 v1.1.6 h1:mqrRot1BRxm+Yct+vavLMou2/iJt0tNVTTC0QoIjaZg=
github.com/twmb/murmur3 v1.1.6/go.mod h1:Qq/R7NUyOfr65zD+6Q5IHKsJLwP7exErjN6lyyq3OSQ=
github.com/twmb/murmur3 v1.1.8 h1:8Yt9taO/WN3l08xErzjeschgZU2QSrwm1kclYq+0aRg=
github.com/twmb/murmur3 v1.1.8/go.mod h1:Qq/R7NUyOfr65zD+6Q5IHKsJLwP7exErjN6lyyq3OSQ=
github.com/uber-go/tally/v4 v4.1.16 h1:by2hveWRh/cUReButk6ns1sHK/hiKry7BuOV6iY16XI=
github.com/uber-go/tally/v4 v4.1.16/go.mod h1:RW5DgqsyEPs0lA4b0YNf4zKj7DveKHd73hnO6zVlyW0=
github.com/valyala/fastjson v1.6.4 h1:uAUNq9Z6ymTgGhcm0UynUAB6tlbakBrz6CQFax3BXVQ=
Expand Down
27 changes: 23 additions & 4 deletions pkg/server/plugin/nodeattestor/awsiid/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (

"github.com/aws/aws-sdk-go-v2/service/ec2"
"github.com/aws/aws-sdk-go-v2/service/iam"
"github.com/aws/aws-sdk-go-v2/service/organizations"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
)
Expand All @@ -18,11 +19,13 @@ var (
type Client interface {
ec2.DescribeInstancesAPIClient
iam.GetInstanceProfileAPIClient
organizations.ListAccountsAPIClient
}

type clientsCache struct {
mtx sync.RWMutex
config *SessionConfig
orgConfig *orgValidationConfig
clients map[string]*cacheEntry
newClient newClientCallback
}
Expand All @@ -32,7 +35,7 @@ type cacheEntry struct {
client Client
}

type newClientCallback func(ctx context.Context, config *SessionConfig, region string, assumeRoleARN string) (Client, error)
type newClientCallback func(ctx context.Context, config *SessionConfig, region string, assumeRoleARN string, orgRoleARN string) (Client, error)

func newClientsCache(newClient newClientCallback) *clientsCache {
return &clientsCache{
Expand All @@ -41,10 +44,11 @@ func newClientsCache(newClient newClientCallback) *clientsCache {
}
}

func (cc *clientsCache) configure(config SessionConfig) {
func (cc *clientsCache) configure(config SessionConfig, orgConfig orgValidationConfig) {
cc.mtx.Lock()
cc.clients = make(map[string]*cacheEntry)
cc.config = &config
cc.orgConfig = &orgConfig
cc.mtx.Unlock()
}

Expand Down Expand Up @@ -81,7 +85,13 @@ func (cc *clientsCache) getClient(ctx context.Context, region, accountID string)
assumeRoleArn = fmt.Sprintf("arn:%s:iam::%s:role/%s", cc.config.Partition, accountID, cc.config.AssumeRole)
}

client, err := cc.newClient(ctx, cc.config, region, assumeRoleArn)
// If organization attestation feature is enabled, assume org role
var orgRoleArn string
if cc.orgConfig.AccountRole != "" {
orgRoleArn = fmt.Sprintf("arn:%s:iam::%s:role/%s", cc.config.Partition, cc.orgConfig.AccountID, cc.orgConfig.AccountRole)
}

client, err := cc.newClient(ctx, cc.config, region, assumeRoleArn, orgRoleArn)
if err != nil {
return nil, status.Errorf(codes.Internal, "failed to create client: %v", err)
}
Expand All @@ -103,16 +113,25 @@ func (cc *clientsCache) getCachedClient(cacheKey string) *cacheEntry {
return r
}

func newClient(ctx context.Context, config *SessionConfig, region string, assumeRoleARN string) (Client, error) {
func newClient(ctx context.Context, config *SessionConfig, region string, assumeRoleARN string, orgRoleArn string) (Client, error) {
conf, err := newAWSConfig(ctx, config.AccessKeyID, config.SecretAccessKey, region, assumeRoleARN)
if err != nil {
return nil, err
}

// If the orgnizationAttestation feature is enabled, use the role configured for feature.
orgConf, err := newAWSConfig(ctx, config.AccessKeyID, config.SecretAccessKey, region, orgRoleArn)
rushi47 marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return nil, err
}

return struct {
iam.GetInstanceProfileAPIClient
ec2.DescribeInstancesAPIClient
organizations.ListAccountsAPIClient
}{
GetInstanceProfileAPIClient: iam.NewFromConfig(conf),
DescribeInstancesAPIClient: ec2.NewFromConfig(conf),
ListAccountsAPIClient: organizations.NewFromConfig(orgConf),
}, nil
}
90 changes: 83 additions & 7 deletions pkg/server/plugin/nodeattestor/awsiid/iid.go
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,8 @@ type IIDAttestorPlugin struct {
mtx sync.RWMutex
clients *clientsCache

orgValidation *orgValidator

// test hooks
hooks struct {
getAWSCACertificate func(string, PublicKeyType) (*x509.Certificate, error)
Expand All @@ -112,12 +114,13 @@ type IIDAttestorPlugin struct {
// IIDAttestorConfig holds hcl configuration for IID attestor plugin
type IIDAttestorConfig struct {
SessionConfig `hcl:",squash"`
SkipBlockDevice bool `hcl:"skip_block_device"`
DisableInstanceProfileSelectors bool `hcl:"disable_instance_profile_selectors"`
LocalValidAcctIDs []string `hcl:"account_ids_for_local_validation"`
AgentPathTemplate string `hcl:"agent_path_template"`
AssumeRole string `hcl:"assume_role"`
Partition string `hcl:"partition"`
SkipBlockDevice bool `hcl:"skip_block_device"`
DisableInstanceProfileSelectors bool `hcl:"disable_instance_profile_selectors"`
LocalValidAcctIDs []string `hcl:"account_ids_for_local_validation"`
AgentPathTemplate string `hcl:"agent_path_template"`
AssumeRole string `hcl:"assume_role"`
Partition string `hcl:"partition"`
ValidateOrgAccountID *orgValidationConfig `hcl:"verify_organization"`
pathTemplate *agentpathtemplate.Template
trustDomain spiffeid.TrustDomain
getAWSCACertificate func(string, PublicKeyType) (*x509.Certificate, error)
Expand All @@ -126,6 +129,7 @@ type IIDAttestorConfig struct {
// New creates a new IIDAttestorPlugin.
func New() *IIDAttestorPlugin {
p := &IIDAttestorPlugin{}
p.orgValidation = newOrganizationValidationBase(&orgValidationConfig{})
p.clients = newClientsCache(defaultNewClientCallback)
p.hooks.getAWSCACertificate = getAWSCACertificate
p.hooks.getenv = os.Getenv
Expand Down Expand Up @@ -154,6 +158,26 @@ func (p *IIDAttestorPlugin) Attest(stream nodeattestorv1.NodeAttestor_AttestServ
return err
}

// Feature account belongs to organization
// Get the account id of the node from attestation and then check if respective account belongs to organization
if c.ValidateOrgAccountID != nil {
ctxValidateOrg, cancel := context.WithTimeout(stream.Context(), awsTimeout)
defer cancel()
orgClient, err := p.clients.getClient(ctxValidateOrg, c.ValidateOrgAccountID.AccountRegion, c.ValidateOrgAccountID.AccountID)
if err != nil {
return status.Errorf(codes.Internal, "failed to get org client: %v", err)
}

valid, err := p.orgValidation.IsMemberAccount(ctxValidateOrg, orgClient, attestationData.AccountID)
if err != nil {
return status.Errorf(codes.Internal, "failed aws ec2 attestation, issue while verifying if nodes account id: %v belong to org: %v", attestationData.AccountID, err)
}

if !valid {
return status.Errorf(codes.Internal, "failed aws ec2 attestation, nodes account id: %v is not part of configured organization or doesn't have ACTIVE status", attestationData.AccountID)
}
}

inTrustAcctList := false
for _, id := range c.LocalValidAcctIDs {
if attestationData.AccountID == id {
Expand Down Expand Up @@ -272,18 +296,35 @@ func (p *IIDAttestorPlugin) Configure(_ context.Context, req *configv1.Configure
return nil, status.Errorf(codes.InvalidArgument, "invalid partition %q, must be one of: %v", config.Partition, partitions)
}

// Check if Feature flag for account belongs to organization is enabled.
orgConfig := &orgValidationConfig{}
Copy link
Collaborator

Choose a reason for hiding this comment

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

why creating a new empty config is required?, why not to use the one from config?
you are already verifying it is not nil when required.
and in case of clients.configure you can validate it is not nil as you are doing here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MarcosDY so i added new variable so that I dont touch existing code too much.
In this case i didnt want to spoil client config
function. As struct is copied in configure function, we will need to perform empty check against the struct and it will look something like below, please lmk which one you prefer or if there is better way to write this validation. And I will change accordingly.

func (cc *clientsCache) configure(config SessionConfig, orgConfig orgValidationConfig) {
	cc.mtx.Lock()
	cc.clients = make(map[string]*cacheEntry)
	cc.config = &config
	
	orgConf := &orgValidationConfig{}
	if orgConf != &orgConfig {
		orgConf = &orgConfig
	}
	cc.orgConfig = orgConf

	cc.mtx.Unlock()
}

if config.ValidateOrgAccountID != nil {
err = validateOrganizationConfig(config)
if err != nil {
return nil, err
}
orgConfig = config.ValidateOrgAccountID
}

p.mtx.Lock()
defer p.mtx.Unlock()

p.config = config
p.clients.configure(config.SessionConfig)
p.clients.configure(config.SessionConfig, *orgConfig)
if config.ValidateOrgAccountID != nil {
// Setup required config, for validation and for bootstrapping org client
if err := p.orgValidation.configure(orgConfig); err != nil {
return nil, err
}
}

return &configv1.ConfigureResponse{}, nil
}

// SetLogger sets this plugin's logger
func (p *IIDAttestorPlugin) SetLogger(log hclog.Logger) {
p.log = log
p.orgValidation.setLogger(log)
}

func (p *IIDAttestorPlugin) checkBlockDevice(instance ec2types.Instance) error {
Expand Down Expand Up @@ -558,3 +599,38 @@ func isValidAWSPartition(partition string) bool {
}
return false
}

func validateOrganizationConfig(config *IIDAttestorConfig) error {
checkAccID := config.ValidateOrgAccountID.AccountID
checkAccRole := config.ValidateOrgAccountID.AccountRole
checkAccRegion := config.ValidateOrgAccountID.AccountRegion

if checkAccID == "" || checkAccRole == "" {
return status.Errorf(codes.InvalidArgument, "please ensure that %q & %q are present inside block or remove the block: %q for feature node attestation using account id verification", orgAccountID, orgAccountRole, "verify_organization")
}

if checkAccRegion == "" {
config.ValidateOrgAccountID.AccountRegion = orgDefaultAccRegion
}

// check TTL if specified
ttl := orgAccountDefaultListDuration
checkTTL := config.ValidateOrgAccountID.AccountListTTL
if checkTTL != "" {
t, err := time.ParseDuration(checkTTL)
if err != nil {
return status.Errorf(codes.InvalidArgument, "please ensure that %q if configured, it should be in duration and is suffixed with required 'm' for time duration in minute ex. '5m'. Otherwise, remove the: %q, in the block: %q. Default TTL will be: %q", orgAccountListTTL, orgAccountListTTL, "verify_organization", orgAccountDefaultListTTL)
}

if t.Minutes() < orgAccountMinTTL.Minutes() {
return status.Errorf(codes.InvalidArgument, "please ensure that %q if configured, it should be greater than or equal to %q. Otherwise remove the: %q, in the block: %q. Default TTL will be: %q", orgAccountListTTL, orgAccountMinListTTL, orgAccountListTTL, "verify_organization", orgAccountDefaultListTTL)
}

ttl = t
}

// Assign default ttl if ttl doesnt exist.
config.ValidateOrgAccountID.AccountListTTL = ttl.String()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can avoid setting this since if its empty we can use default that will be already a duration,
and when invalid it will fail.
Another thing we can do here is not use AccountListTTL but store the duration we already parsed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can avoid setting this since if its empty we can use default that will be already a duration,
and when invalid it will fail.

Would you mind elaborating more on it ? I am thinking if we don't set it here, when we access this configuration we will need to do check for empty or parse this string everytime.

Another thing we can do here is not use AccountListTTL but store the duration we already parsed.

Right, yes we do this already once we know the config is clean, setup defaults wherever required.
So when client is configured we set this in duration

Copy link
Collaborator

Choose a reason for hiding this comment

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

since ttl was already parsed in

		t, err := time.ParseDuration(checkTTL)

why to keep this as an string?
string will be used only for logs, but duration is the one used in code,
and you will need to parse this again after validation...
so why not to parse it once and keep it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so the function validateOrganizationConfig all it's doing is just verifying/cleaning the configuration passed and adding defaults where required.

Now in this case as the object passed to this functions is instance of config which was read as String from the config file, am reassigning the default in string format.

you will need to parse this again after validation...
so why not to parse it once and keep it??

Thats correct, we parse it just once again after this & then avoid parsing it again & again as we instantiate the instance of this verification method, its done here.

Please lmk if this answers your concern.


return nil
}
Loading
Loading