Add support for multiple public certificates to AWS IID node attestor #4124
Conversation
maxlambrecht
requested review from
evan2645,
amartinezfayo,
azdagron,
MarcosDY and
rturner3
as code owners
| func getAWSCACertificate(region string, keyType PublicKeyType) (*x509.Certificate, error) { | ||
| var cert string | ||
| if keyType == KeyTypeUnset { | ||
| return nil, fmt.Errorf("signature type is unset") |
There was a problem hiding this comment.
| return nil, fmt.Errorf("signature type is unset") | |
| return nil, fmt.Errorf("signature key type is unset") |
| @@ -223,14 +227,10 @@ func (p *IIDAttestorPlugin) Configure(ctx context.Context, req *configv1.Configu | |||
| return nil, status.Errorf(codes.InvalidArgument, "unable to decode configuration: %v", err) | |||
| } | |||
|
|
|||
| // Get the AWS CA public key. We do this lazily on configure so deployments | |||
| // Function to get the AWS CA certificate. We do this lazily on configure so deployments | |||
There was a problem hiding this comment.
Did you measure the startup cost if we parse all of the certificates in advance? I'd be surprised if the impact was substantial. I'd also much prefer to pay that cost once instead of every time we attest a new node. My vote would be to either 1) just parse them all on startup, or 2) cache them once parsed so we don't pay the cost continuously.
There was a problem hiding this comment.
I prefer option 2. Rather than parsing all 45 certificates on startup, I'd choose parsing and caching the certificates on demand. This way, only the certificates that are actually used will be parsed and cached, avoiding unnecessary overhead.
| @@ -106,14 +110,14 @@ type IIDAttestorConfig struct { | |||
| AssumeRole string `hcl:"assume_role"` | |||
| pathTemplate *agentpathtemplate.Template | |||
| trustDomain spiffeid.TrustDomain | |||
| awsCAPublicKey *rsa.PublicKey | |||
| getAWSCACertificate func(string, PublicKeyType) (*x509.Certificate, error) | |||
There was a problem hiding this comment.
We talked about maybe providing a way in the config for operators to specify an override for the signing certificate for a given region. Is that not needed anymore due to the very long expiration dates on the RSA2048 CAs?
There was a problem hiding this comment.
We talked about providing a way to specify a cert for region, and my understanding is that we prefer to have all the certs and select automatically which one to use based on the region, which I think is not bad idea considering the very long expiration dates.
| var attestationData caws.IIDAttestationData | ||
| var doc imds.InstanceIdentityDocument |
There was a problem hiding this comment.
nitpick: variables should be defined closer to the point of use
| @@ -56,8 +60,14 @@ var ( | |||
| ebsType = ec2types.DeviceTypeEbs | |||
| ) | |||
|
|
|||
| var testAWSCACert *x509.Certificate | |||
There was a problem hiding this comment.
nitpick: these two vars should go in the var block above
Signed-off-by: Max Lambrecht <maxlambrecht@gmail.com>
Signed-off-by: Max Lambrecht <maxlambrecht@gmail.com>
Signed-off-by: Max Lambrecht <maxlambrecht@gmail.com>
Signed-off-by: Max Lambrecht <maxlambrecht@gmail.com>
Signed-off-by: Max Lambrecht <maxlambrecht@gmail.com>
maxlambrecht
force-pushed
the
aws_iid_attestor_multiple_certs_support
branch
from
89a43c5
to
2bfd1fe
Compare
…spiffe#4124) Signed-off-by: Max Lambrecht <maxlambrecht@gmail.com> Signed-off-by: Neniel <11655196+Neniel@users.noreply.github.com>
Pull Request check list
Affected functionality
Add support for all regions to the
AWS_IIDNode Attestor.Description of change
Backward compatibility
The SPIRE Server also supports the use of RSA-1024 certificates: https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/verify-signature.html
The "default" certificate is the one currently used by SPIRE, which expires on Jun-2024. The certificates for specific regions were also added as part of this PR.
SPIRE Agent will send both type of signatures: the signature from the path
instance-identity/signature, that can be validated using RSA-1024 certificates); and the the signature from the pathinstance-identity/rsa2048, that can be validated using RSA-2048 certificates).SPIRE Server will use the RSA-2048 signature if present, if not, will fall back to RSA-1024.
In a nutshell:
New SPIRE Server attests new SPIRE Agents, using RSA-2048 certificates and signature.
Old SPIRE Server attests new SPIRE Agents, using only one "default" RSA-1024 certificate, supporting only a set of regions.
New SPIRE Server attests old SPIRE Agents, using RSA-1024 certificates, supporting all regions.
Which issue this PR fixes
Fixes #2126