From c054b0213eb118c5d394c201a51e0bd6f40482fe Mon Sep 17 00:00:00 2001 From: Julian Sommer <115516918+juliansommer@users.noreply.github.com> Date: Wed, 22 Jul 2026 16:42:38 +1000 Subject: [PATCH] fix(ec2): render ED25519 key fingerprints as EC2 does (#571) --- spinifex/handlers/ec2/key/metadata.go | 75 ++++++++++ spinifex/handlers/ec2/key/metadata_test.go | 124 ++++++++++++++++ spinifex/handlers/ec2/key/record_tags_test.go | 48 +++++-- spinifex/handlers/ec2/key/service_impl.go | 82 ++++++----- .../handlers/ec2/key/service_impl_test.go | 136 ++++++++++++------ 5 files changed, 377 insertions(+), 88 deletions(-) create mode 100644 spinifex/handlers/ec2/key/metadata.go create mode 100644 spinifex/handlers/ec2/key/metadata_test.go diff --git a/spinifex/handlers/ec2/key/metadata.go b/spinifex/handlers/ec2/key/metadata.go new file mode 100644 index 000000000..3e8548cc9 --- /dev/null +++ b/spinifex/handlers/ec2/key/metadata.go @@ -0,0 +1,75 @@ +package handlers_ec2_key + +import ( + "crypto/sha256" + "encoding/base64" + "encoding/json" + "strings" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" +) + +// openSSHSHA256Prefix is what ssh.FingerprintSHA256 puts in front of the digest. +// EC2 never emits it, so a stored fingerprint carrying it was written by the +// earlier code that rendered ED25519 keys the OpenSSH way. +const openSSHSHA256Prefix = "SHA256:" + +// keyPairMetadata is the stored form of a key pair, written to +// keys//.json. The JSON keys match the field names +// ec2.CreateKeyPairOutput marshalled under, which this record was serialised as +// before it had a type of its own, so records already on disk still decode. An +// empty KeyType is exactly that case, and is what triggers the legacy upgrade in +// decodeKeyPairMetadata. +type keyPairMetadata struct { + KeyFingerprint *string `json:"KeyFingerprint"` + KeyName *string `json:"KeyName"` + KeyPairId *string `json:"KeyPairId"` + KeyType string `json:"KeyType,omitempty"` + Tags []*ec2.Tag `json:"Tags"` +} + +// decodeKeyPairMetadata unmarshals a stored record and upgrades it to current +// form. Records written before KeyType was persisted are typed by the "SHA256:" +// prefix their ED25519 fingerprints still carry, and that fingerprint is then +// re-rendered in the AWS form. Resolving the type before rewriting the value it +// was inferred from is what keeps the upgrade lossless. +func decodeKeyPairMetadata(body []byte) (*keyPairMetadata, error) { + var metadata keyPairMetadata + if err := json.Unmarshal(body, &metadata); err != nil { + return nil, err + } + + // A record that names its own type was written by code that also renders + // fingerprints as EC2 does, so there is nothing to upgrade. + if metadata.KeyType != "" { + return &metadata, nil + } + + // Only ED25519 was ever stored prefixed; every other legacy fingerprint is + // the colon hex of an RSA digest and is left exactly as it was found. + if metadata.KeyFingerprint != nil && strings.HasPrefix(*metadata.KeyFingerprint, openSSHSHA256Prefix) { + metadata.KeyType = "ed25519" + metadata.KeyFingerprint = aws.String(awsFingerprint(*metadata.KeyFingerprint)) + } else { + metadata.KeyType = "rsa" + } + + return &metadata, nil +} + +// awsFingerprint re-renders an OpenSSH SHA-256 fingerprint the way EC2 spells +// it: the "SHA256:" prefix dropped and the base64 padding OpenSSH omits +// restored. Anything that is not a raw base64 SHA-256 digest is corruption +// rather than a legacy rendering, and is returned verbatim instead of guessed +// at. The length check is what makes that true of a truncated value: a short +// body still decodes cleanly, so without it a record holding "SHA256:" alone +// would re-render as the empty string and the next write would persist that +// over the stored value. +func awsFingerprint(fingerprint string) string { + digest, err := base64.RawStdEncoding.DecodeString(strings.TrimPrefix(fingerprint, openSSHSHA256Prefix)) + if err != nil || len(digest) != sha256.Size { + return fingerprint + } + return base64.StdEncoding.EncodeToString(digest) +} diff --git a/spinifex/handlers/ec2/key/metadata_test.go b/spinifex/handlers/ec2/key/metadata_test.go new file mode 100644 index 000000000..5d177d69d --- /dev/null +++ b/spinifex/handlers/ec2/key/metadata_test.go @@ -0,0 +1,124 @@ +package handlers_ec2_key + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The two ED25519 fingerprints AWS itself returned on 2026-07-22 (account +// 744090693897, us-east-1), one per write path. They are recorded observations, +// not derivations, and they pin the rendering without needing the key material +// that produced them. +const ( + awsImportedED25519Fingerprint = "8ZVMcTKqdR5hxMjZI3t6b09EPIOE5QA6G9/Z63ejNBM=" + awsCreatedED25519Fingerprint = "odzmoB4gyxi1jn9vGD4c+bOpt1XUX1JUyaHaWHHCO2w=" +) + +// A legacy ED25519 record is typed from the prefix and then re-rendered as the +// AWS values above spell it. Each stored value is the OpenSSH rendering of the +// very digest AWS reported for that key pair. +func TestDecodeKeyPairMetadata_LegacyED25519(t *testing.T) { + tests := []struct { + name string + stored string + want string + }{ + { + name: "imported", + stored: "SHA256:8ZVMcTKqdR5hxMjZI3t6b09EPIOE5QA6G9/Z63ejNBM", + want: awsImportedED25519Fingerprint, + }, + { + name: "created", + stored: "SHA256:odzmoB4gyxi1jn9vGD4c+bOpt1XUX1JUyaHaWHHCO2w", + want: awsCreatedED25519Fingerprint, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + metadata, err := decodeKeyPairMetadata([]byte(`{"KeyFingerprint":"` + tt.stored + `","KeyName":"k","KeyPairId":"key-1"}`)) + require.NoError(t, err) + assert.Equal(t, "ed25519", metadata.KeyType) + assert.Equal(t, tt.want, *metadata.KeyFingerprint) + }) + } +} + +// A record that names its own type is left exactly as stored, whatever its +// fingerprint looks like: it was written by code that already renders as EC2 +// does, so re-deriving either field could only lose information. +func TestDecodeKeyPairMetadata_KeyTypeWins(t *testing.T) { + tests := []struct { + name string + keyType string + fingerprint string + }{ + {name: "ED25519", keyType: "ed25519", fingerprint: awsImportedED25519Fingerprint}, + {name: "RSA", keyType: "rsa", fingerprint: testRSAImportedFingerprint}, + {name: "PrefixedNotRewritten", keyType: "ed25519", fingerprint: testLegacyED25519Fingerprint}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + metadata, err := decodeKeyPairMetadata([]byte( + `{"KeyFingerprint":"` + tt.fingerprint + `","KeyName":"k","KeyPairId":"key-1","KeyType":"` + tt.keyType + `"}`)) + require.NoError(t, err) + assert.Equal(t, tt.keyType, metadata.KeyType) + assert.Equal(t, tt.fingerprint, *metadata.KeyFingerprint) + }) + } +} + +// Legacy RSA records carry no prefix, so they type as rsa and their colon hex is +// left untouched -- at both digest widths, 16 bytes imported and 20 created. +func TestDecodeKeyPairMetadata_LegacyRSA(t *testing.T) { + for name, fingerprint := range map[string]string{ + "imported": testRSAImportedFingerprint, + "created": testRSACreatedFingerprint, + } { + t.Run(name, func(t *testing.T) { + metadata, err := decodeKeyPairMetadata([]byte(`{"KeyFingerprint":"` + fingerprint + `","KeyName":"k","KeyPairId":"key-1"}`)) + require.NoError(t, err) + assert.Equal(t, "rsa", metadata.KeyType) + assert.Equal(t, fingerprint, *metadata.KeyFingerprint) + }) + } +} + +// A prefixed value that is not a SHA-256 digest is corruption, not a legacy +// rendering, and is reported back verbatim so the corruption stays visible. The +// truncated cases are the ones that matter: they decode cleanly, so nothing but +// the digest length distinguishes them from a value worth re-rendering, and +// re-rendering them would persist the mangled result on the next tag write. +func TestDecodeKeyPairMetadata_CorruptFingerprint(t *testing.T) { + for name, fingerprint := range map[string]string{ + "NotBase64": "SHA256:not valid base64!", + "AlreadyPadded": openSSHSHA256Prefix + testED25519Fingerprint, + "EmptyBody": openSSHSHA256Prefix, + "TruncatedBody": "SHA256:+DiY3wvv", + } { + t.Run(name, func(t *testing.T) { + metadata, err := decodeKeyPairMetadata([]byte(`{"KeyFingerprint":"` + fingerprint + `","KeyName":"k","KeyPairId":"key-1"}`)) + require.NoError(t, err) + assert.Equal(t, "ed25519", metadata.KeyType) + assert.Equal(t, fingerprint, *metadata.KeyFingerprint) + }) + } +} + +// A record with no fingerprint at all must not be dereferenced on the way to a +// key type. +func TestDecodeKeyPairMetadata_NoFingerprint(t *testing.T) { + metadata, err := decodeKeyPairMetadata([]byte(`{"KeyName":"k","KeyPairId":"key-1"}`)) + require.NoError(t, err) + assert.Equal(t, "rsa", metadata.KeyType) + assert.Nil(t, metadata.KeyFingerprint) +} + +func TestDecodeKeyPairMetadata_InvalidJSON(t *testing.T) { + _, err := decodeKeyPairMetadata([]byte("not json{{{")) + require.Error(t, err) +} diff --git a/spinifex/handlers/ec2/key/record_tags_test.go b/spinifex/handlers/ec2/key/record_tags_test.go index 358ee9dfe..c1935c357 100644 --- a/spinifex/handlers/ec2/key/record_tags_test.go +++ b/spinifex/handlers/ec2/key/record_tags_test.go @@ -4,32 +4,31 @@ import ( "context" "encoding/json" "fmt" - "io" "testing" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" - "github.com/aws/aws-sdk-go/service/s3" "github.com/mulgadc/spinifex/spinifex/filterutil" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +// The record is read back as ec2.CreateKeyPairOutput, the type it was +// serialised as before keyPairMetadata existed, so this doubles as a canary on +// the stored JSON keys not having moved. func readKeyPairTags(t *testing.T, svc *KeyServiceImpl, accountID, keyPairID string) map[string]string { t.Helper() - result, err := svc.store.GetObject(context.Background(), &s3.GetObjectInput{ - Bucket: aws.String(testBucket), - Key: aws.String(fmt.Sprintf("keys/%s/%s.json", accountID, keyPairID)), - }) - require.NoError(t, err) - defer result.Body.Close() - body, err := io.ReadAll(result.Body) - require.NoError(t, err) var metadata ec2.CreateKeyPairOutput - require.NoError(t, json.Unmarshal(body, &metadata)) + require.NoError(t, json.Unmarshal(readKeyPairRecord(t, svc, accountID, keyPairID), &metadata)) return filterutil.EC2TagsToMap(metadata.Tags) } +// readKeyPairRecord returns the stored metadata object verbatim. +func readKeyPairRecord(t *testing.T, svc *KeyServiceImpl, accountID, keyPairID string) []byte { + t.Helper() + return readStoredObject(t, svc.store, fmt.Sprintf("keys/%s/%s.json", accountID, keyPairID)) +} + func TestKeyPairRecordTagsMirror(t *testing.T) { svc, _ := newTestKeyService() out := importTestKey(t, svc, "tag-mirror-key") @@ -77,6 +76,33 @@ func TestKeyPairRecordTagsMirror_CrossAccountNoMutation(t *testing.T) { assert.Empty(t, tags) } +// Tagging read-modify-writes the whole record, so a legacy one is rewritten in +// upgraded form. The two halves of that upgrade must travel together: writing +// the normalised fingerprint back without also persisting KeyType would strip +// the only evidence of the key's type and permanently downgrade it to rsa. +func TestKeyPairRecordTagsMirror_UpgradesLegacyRecord(t *testing.T) { + svc, store := newTestKeyService() + + putStoredObject(t, store, "keys/"+testAccountID+"/key-legacy2.json", + `{"KeyFingerprint":"`+testLegacyED25519Fingerprint+`","KeyName":"legacy-tagged","KeyPairId":"key-legacy2","Tags":null}`) + + require.NoError(t, svc.ApplyRecordTags(&ec2.CreateTagsInput{ + Resources: []*string{aws.String("key-legacy2")}, + Tags: []*ec2.Tag{{Key: aws.String("env"), Value: aws.String("prod")}}, + }, testAccountID)) + + record := string(readKeyPairRecord(t, svc, testAccountID, "key-legacy2")) + assert.Contains(t, record, `"KeyType":"ed25519"`) + assert.Contains(t, record, `"KeyFingerprint":"`+testED25519Fingerprint+`"`) + + out, err := svc.DescribeKeyPairs(context.Background(), &ec2.DescribeKeyPairsInput{}, testAccountID) + require.NoError(t, err) + require.Len(t, out.KeyPairs, 1) + assert.Equal(t, "ed25519", *out.KeyPairs[0].KeyType) + assert.Equal(t, testED25519Fingerprint, *out.KeyPairs[0].KeyFingerprint) + assert.Equal(t, "prod", filterutil.EC2TagsToMap(out.KeyPairs[0].Tags)["env"]) +} + func TestKeyPairRecordTagsMirror_UnownedNoError(t *testing.T) { svc, _ := newTestKeyService() require.NoError(t, svc.ApplyRecordTags(&ec2.CreateTagsInput{ diff --git a/spinifex/handlers/ec2/key/service_impl.go b/spinifex/handlers/ec2/key/service_impl.go index d4b54a722..5292775d5 100644 --- a/spinifex/handlers/ec2/key/service_impl.go +++ b/spinifex/handlers/ec2/key/service_impl.go @@ -5,7 +5,9 @@ import ( "context" "crypto/md5" //nolint:gosec // G501: MD5 is the digest EC2 puts on the wire, not a security choice "crypto/sha1" //nolint:gosec // G505: SHA-1 is the digest EC2 puts on the wire, not a security choice + "crypto/sha256" "crypto/x509" + "encoding/base64" "encoding/hex" "encoding/json" "errors" @@ -159,6 +161,18 @@ func (s *KeyServiceImpl) CreateKeyPair(ctx context.Context, input *ec2.CreateKey return nil, errors.New(awserrors.ErrorServerInternal) } + // From here keyType describes the key that exists rather than the one that + // was asked for, so that the type stored alongside the fingerprint is read + // off the same key the fingerprint was taken from. The import path types its + // keys the same way. createdKeyFingerprint has already rejected every + // algorithm this can refuse, so the error is unreachable and stated only so + // an unsupported key can never be stored under a type EC2 cannot report. + keyType, err = keyPairType(publicKey) + if err != nil { + slog.ErrorContext(ctx, "Generated key has an unsupported algorithm", "algorithm", publicKey.Type(), "keyName", keyName, "err", err) + return nil, errors.New(awserrors.ErrorServerInternal) + } + // Upload public key to S3 _, err = s.store.PutObject(ctx, &s3.PutObjectInput{ Bucket: aws.String(s.bucketName), @@ -181,11 +195,12 @@ func (s *KeyServiceImpl) CreateKeyPair(ctx context.Context, input *ec2.CreateKey Tags: tags, } - // Store metadata file (CreateKeyPairOutput without KeyMaterial) for keyPairId lookups - err = s.storeKeyPairMetadata(ctx, accountID, keyPairID, &ec2.CreateKeyPairOutput{ + // Store metadata file (everything but the private key) for keyPairId lookups + err = s.storeKeyPairMetadata(ctx, accountID, keyPairID, &keyPairMetadata{ KeyFingerprint: aws.String(fingerprint), KeyName: aws.String(keyName), KeyPairId: aws.String(keyPairID), + KeyType: keyType, Tags: tags, }) if err != nil { @@ -206,13 +221,12 @@ func (s *KeyServiceImpl) CreateKeyPair(ctx context.Context, input *ec2.CreateKey } // importedKeyFingerprint fingerprints a key supplied to ImportKeyPair: the MD5 -// of the DER SubjectPublicKeyInfo for RSA, matching AWS, and the OpenSSH SHA-256 -// rendering for ED25519, which does not -- AWS omits the "SHA256:" prefix and -// pads the base64. +// of the DER SubjectPublicKeyInfo for RSA, and the SHA-256 of the SSH wire blob +// for ED25519, both matching AWS. func importedKeyFingerprint(publicKey ssh.PublicKey) (string, error) { switch publicKey.Type() { case ssh.KeyAlgoED25519: - return ssh.FingerprintSHA256(publicKey), nil + return ed25519Fingerprint(publicKey), nil case ssh.KeyAlgoRSA: // AWS hashes the DER SubjectPublicKeyInfo, not the RFC 4253 wire blob @@ -241,14 +255,14 @@ func importedKeyFingerprint(publicKey ssh.PublicKey) (string, error) { // createdKeyFingerprint fingerprints a key EC2 generated itself. For RSA that is // the SHA-1 of the DER PKCS#8 private key, a 20-byte digest over material the // caller receives once and the store never keeps. ED25519 hashes the public key, -// exactly as the import path does, and shares its divergence from AWS. +// exactly as the import path does. func createdKeyFingerprint(privateKeyPEM []byte, publicKey ssh.PublicKey) (string, error) { switch publicKey.Type() { // Matched ahead of the PKCS#8 path deliberately: ParseRawPrivateKey hands // back *ed25519.PrivateKey, a pointer type MarshalPKCS8PrivateKey rejects, // so an ED25519 key reaching that path would fail at runtime. case ssh.KeyAlgoED25519: - return ssh.FingerprintSHA256(publicKey), nil + return ed25519Fingerprint(publicKey), nil case ssh.KeyAlgoRSA: rawKey, err := ssh.ParseRawPrivateKey(privateKeyPEM) @@ -268,6 +282,14 @@ func createdKeyFingerprint(privateKeyPEM []byte, publicKey ssh.PublicKey) (strin } } +// ed25519Fingerprint renders the SHA-256 of the SSH wire blob as EC2 spells it: +// bare padded base64, without the "SHA256:" prefix OpenSSH prepends and with the +// padding OpenSSH omits. +func ed25519Fingerprint(publicKey ssh.PublicKey) string { + sum := sha256.Sum256(publicKey.Marshal()) + return base64.StdEncoding.EncodeToString(sum[:]) +} + // colonHex renders digest bytes the way EC2 spells a fingerprint: lowercase hex // byte pairs joined by colons. func colonHex(digest []byte) string { @@ -295,7 +317,7 @@ func keyPairType(publicKey ssh.PublicKey) (string, error) { } // storeKeyPairMetadata stores key pair metadata (without private key) to S3 for keyPairId lookups. -func (s *KeyServiceImpl) storeKeyPairMetadata(ctx context.Context, accountID, keyPairID string, metadata *ec2.CreateKeyPairOutput) error { +func (s *KeyServiceImpl) storeKeyPairMetadata(ctx context.Context, accountID, keyPairID string, metadata *keyPairMetadata) error { // Store metadata with keyPairId as filename for efficient lookup when keyPairId is provided metadataPath := fmt.Sprintf("keys/%s/%s.json", accountID, keyPairID) @@ -343,8 +365,8 @@ func (s *KeyServiceImpl) getKeyNameFromKeyPairId(ctx context.Context, accountID, return "", fmt.Errorf("failed to read metadata: %w", err) } - var metadata ec2.CreateKeyPairOutput - if err := json.Unmarshal(body, &metadata); err != nil { + metadata, err := decodeKeyPairMetadata(body) + if err != nil { slog.Error("Failed to unmarshal metadata", "err", err) return "", fmt.Errorf("failed to unmarshal metadata: %w", err) } @@ -402,8 +424,8 @@ func (s *KeyServiceImpl) findKeyPairIdFromKeyName(ctx context.Context, accountID continue } - var metadata ec2.CreateKeyPairOutput - if err := json.Unmarshal(body, &metadata); err != nil { + metadata, err := decodeKeyPairMetadata(body) + if err != nil { slog.Debug("Failed to unmarshal metadata", "key", *obj.Key, "err", err) continue } @@ -628,8 +650,8 @@ func (s *KeyServiceImpl) DescribeKeyPairs(ctx context.Context, input *ec2.Descri continue } - var metadata ec2.CreateKeyPairOutput - if err := json.Unmarshal(body, &metadata); err != nil { + metadata, err := decodeKeyPairMetadata(body) + if err != nil { slog.DebugContext(ctx, "Failed to unmarshal metadata", "key", *obj.Key, "err", err) continue } @@ -662,20 +684,15 @@ func (s *KeyServiceImpl) DescribeKeyPairs(ctx context.Context, input *ec2.Descri } } - // Determine key type from fingerprint format - var keyType string - if metadata.KeyFingerprint != nil && strings.HasPrefix(*metadata.KeyFingerprint, "SHA256:") { - keyType = "ed25519" - } else { - keyType = "rsa" - } - - // Build KeyPairInfo from metadata + // Build KeyPairInfo from metadata. Records predating the stored KeyType + // were typed by the decoder, which also normalised the fingerprint they + // were typed from; the object itself is left as it was found, so a list + // does not turn into a write per key pair. keyPairInfo := &ec2.KeyPairInfo{ KeyPairId: metadata.KeyPairId, KeyFingerprint: metadata.KeyFingerprint, KeyName: metadata.KeyName, - KeyType: aws.String(keyType), + KeyType: aws.String(metadata.KeyType), Tags: metadata.Tags, } @@ -836,14 +853,13 @@ func (s *KeyServiceImpl) ImportKeyPair(ctx context.Context, input *ec2.ImportKey } // Store metadata file (without public key material) - metadataOutput := &ec2.CreateKeyPairOutput{ + err = s.storeKeyPairMetadata(ctx, accountID, keyPairID, &keyPairMetadata{ KeyFingerprint: aws.String(fingerprint), KeyName: aws.String(keyName), KeyPairId: aws.String(keyPairID), + KeyType: keyType, Tags: tags, - } - - err = s.storeKeyPairMetadata(ctx, accountID, keyPairID, metadataOutput) + }) if err != nil { slog.ErrorContext(ctx, "Failed to store key pair metadata", "err", err, "keyPairId", keyPairID) // Try to cleanup the public key we just uploaded @@ -905,8 +921,10 @@ func (s *KeyServiceImpl) mirrorKeyPairTags(ctx context.Context, resources []*str if err != nil { return fmt.Errorf("failed to read key pair metadata: %w", err) } - var metadata ec2.CreateKeyPairOutput - if err := json.Unmarshal(body, &metadata); err != nil { + // The whole record is rewritten, not just its Tags, so a legacy record is + // persisted here in the form the decoder upgraded it to. + metadata, err := decodeKeyPairMetadata(body) + if err != nil { return fmt.Errorf("failed to unmarshal key pair metadata: %w", err) } tags := filterutil.EC2TagsToMap(metadata.Tags) @@ -915,7 +933,7 @@ func (s *KeyServiceImpl) mirrorKeyPairTags(ctx context.Context, resources []*str } mut(tags) metadata.Tags = utils.MapToEC2Tags(tags) - if err := s.storeKeyPairMetadata(ctx, accountID, *res, &metadata); err != nil { + if err := s.storeKeyPairMetadata(ctx, accountID, *res, metadata); err != nil { return err } } diff --git a/spinifex/handlers/ec2/key/service_impl_test.go b/spinifex/handlers/ec2/key/service_impl_test.go index e48f654f0..dca4847a7 100644 --- a/spinifex/handlers/ec2/key/service_impl_test.go +++ b/spinifex/handlers/ec2/key/service_impl_test.go @@ -3,8 +3,11 @@ package handlers_ec2_key import ( "context" "crypto/sha1" + "crypto/sha256" "crypto/x509" + "encoding/base64" "encoding/json" + "io" "os/exec" "strings" "testing" @@ -38,9 +41,16 @@ const testECDSAPubKey = "ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAA const testDSAPubKey = "ssh-dss AAAAB3NzaC1kc3MAAACBAMfooAGeGkAp6syFsxNueaASpwMr5t+BiLTxAmf9grlH/7MPhXHxxtrrzEdq4bK0mheLl4irAtardgR5ghOVxKXqz4dLlcEyv3H3tOY8Hq+JT/6w9j4FLjen4obXcilh7vfRPdL/A7Dk3Th9NSkrp43FmXUL4EyuMbqi7LcQYpMpAAAAFQDpwLJZvztWN3pEeT/MMLsVSmJedwAAAIBZjhEyHHk1iomvueP6GkmdrXt4V9+6BHjG/rHzQRlO79muU5ImX/BFALCc0RjaPNAoo0lF6ptaPf2HPeu3dtEAWM9iXH8SLqcAVX7B5FUYKFb7zsyQmlT3pKo21V3mCakKHDma8kbHSC2sysl1NOD4IkGTQalP4MuzIvCXNKbCdAAAAIEAl7OdP7hBngX0CuM0+cJXonZvnvIo1NOWGVu+dCn93mvoGjKFyZmLSEMIfFbmckQF2J4F9cM9aU6ht76k73DFnnA/F7WiJ+hIKLhL7Y8F0eDtWkawswvwxvHB+C7drrqezD6t5INX4CYlNQD4zqhgWKBSKVn3sQzQI95gFE51rKE=" const ( - // The OpenSSH SHA256 rendering, as reported by `ssh-keygen -lf`. This is not - // the value AWS returns: AWS drops the "SHA256:" prefix and pads the base64. - testED25519Fingerprint = "SHA256:+DiY3wvvV6TuJJhbpZisF/zLDA0zPMSvHdkr4UvCOqU" + // The SHA256 digest `ssh-keygen -lf` reports for testED25519PubKey, rendered + // as AWS renders it: no "SHA256:" prefix and the base64 padded. Derived from + // the fixture rather than captured, since the ED25519 key pairs AWS was + // sampled against have been deleted; the AWS captures pin the rendering in + // metadata_test.go instead, where no key material is needed. + testED25519Fingerprint = "+DiY3wvvV6TuJJhbpZisF/zLDA0zPMSvHdkr4UvCOqU=" + + // The same digest as records already in the object store still spell it: the + // OpenSSH rendering `ssh.FingerprintSHA256` produced. + testLegacyED25519Fingerprint = "SHA256:+DiY3wvvV6TuJJhbpZisF/zLDA0zPMSvHdkr4UvCOqU" // Recorded from real AWS: the value ImportKeyPair returned for testRSAPubKey // above. Reproducible with @@ -96,6 +106,32 @@ func newTestKeyService() (*KeyServiceImpl, *objectstore.MemoryObjectStore) { return svc, store } +// readStoredObject returns the body of an object the test asserts is present. +func readStoredObject(t *testing.T, store objectstore.ObjectStore, key string) []byte { + t.Helper() + result, err := store.GetObject(context.Background(), &s3.GetObjectInput{ + Bucket: aws.String(testBucket), + Key: aws.String(key), + }) + require.NoError(t, err) + defer result.Body.Close() + body, err := io.ReadAll(result.Body) + require.NoError(t, err) + return body +} + +// putStoredObject writes a body verbatim, for seeding records in a form the +// service no longer writes itself. +func putStoredObject(t *testing.T, store objectstore.ObjectStore, key, body string) { + t.Helper() + _, err := store.PutObject(context.Background(), &s3.PutObjectInput{ + Bucket: aws.String(testBucket), + Key: aws.String(key), + Body: strings.NewReader(body), + }) + require.NoError(t, err) +} + // requireSSHKeygen skips the test if ssh-keygen is not available. func requireSSHKeygen(t *testing.T) { t.Helper() @@ -133,28 +169,22 @@ func TestCreateKeyPair_ED25519(t *testing.T) { assert.Equal(t, "my-ed25519-key", *out.KeyName) assert.NotEmpty(t, *out.KeyPairId) assert.True(t, strings.HasPrefix(*out.KeyPairId, "key-")) - assert.NotEmpty(t, *out.KeyFingerprint) - assert.True(t, strings.HasPrefix(*out.KeyFingerprint, "SHA256:"), "ed25519 fingerprint should be SHA256 format") assert.NotEmpty(t, *out.KeyMaterial) assert.Contains(t, *out.KeyMaterial, "PRIVATE KEY") // Verify public key stored in S3 keyPath := "keys/" + testAccountID + "/my-ed25519-key" - getOut, err := store.GetObject(context.Background(), &s3.GetObjectInput{ - Bucket: aws.String(testBucket), - Key: aws.String(keyPath), - }) - require.NoError(t, err) - assert.NotNil(t, getOut) + publicKeyData := readStoredObject(t, store, keyPath) - // Verify metadata stored in S3 + // The key is generated per-run, so the digest is not knowable in advance. + // Recompute it from the stored public key: that pins the rendering to an + // independent computation rather than to the shape of the string. + sum := sha256.Sum256(parseTestPubKey(t, string(publicKeyData)).Marshal()) + assert.Equal(t, base64.StdEncoding.EncodeToString(sum[:]), *out.KeyFingerprint) + + // Verify metadata stored in S3, carrying the key type it was created with. metaPath := "keys/" + testAccountID + "/" + *out.KeyPairId + ".json" - metaOut, err := store.GetObject(context.Background(), &s3.GetObjectInput{ - Bucket: aws.String(testBucket), - Key: aws.String(metaPath), - }) - require.NoError(t, err) - assert.NotNil(t, metaOut) + assert.Contains(t, string(readStoredObject(t, store, metaPath)), `"KeyType":"ed25519"`) } func TestCreateKeyPair_RSA(t *testing.T) { @@ -263,21 +293,11 @@ func TestImportKeyPair_Success_ED25519(t *testing.T) { // Verify public key stored in S3 keyPath := "keys/" + testAccountID + "/imported-ed25519" - getOut, err := store.GetObject(context.Background(), &s3.GetObjectInput{ - Bucket: aws.String(testBucket), - Key: aws.String(keyPath), - }) - require.NoError(t, err) - assert.NotNil(t, getOut) + assert.Equal(t, testED25519PubKey, string(readStoredObject(t, store, keyPath))) - // Verify metadata stored in S3 + // Verify metadata stored in S3, carrying the key type it was imported as. metaPath := "keys/" + testAccountID + "/" + *out.KeyPairId + ".json" - metaOut, err := store.GetObject(context.Background(), &s3.GetObjectInput{ - Bucket: aws.String(testBucket), - Key: aws.String(metaPath), - }) - require.NoError(t, err) - assert.NotNil(t, metaOut) + assert.Contains(t, string(readStoredObject(t, store, metaPath)), `"KeyType":"ed25519"`) } func TestImportKeyPair_Success_RSA(t *testing.T) { @@ -571,23 +591,27 @@ func TestDescribeKeyPairs_AllKeys(t *testing.T) { assert.True(t, names["key-beta"]) } -// KeyType is inferred from the stored fingerprint's shape, so it has to survive -// RSA reporting two different digest widths: 16 bytes when imported, 20 when EC2 -// generated the key. Only ED25519 carries the "SHA256:" prefix that marks it. -func TestDescribeKeyPairs_KeyTypeInference(t *testing.T) { +// KeyType is now read back from the record rather than guessed at, across every +// combination of path and algorithm that writes one. +func TestDescribeKeyPairs_KeyType(t *testing.T) { requireSSHKeygen(t) svc, _ := newTestKeyService() - importTestKey(t, svc, "inferred-ed25519") + importTestKey(t, svc, "ed25519-imported") _, err := svc.ImportKeyPair(context.Background(), &ec2.ImportKeyPairInput{ - KeyName: aws.String("inferred-rsa-imported"), + KeyName: aws.String("rsa-imported"), PublicKeyMaterial: []byte(testRSAPubKey), }, testAccountID) require.NoError(t, err) _, err = svc.CreateKeyPair(context.Background(), &ec2.CreateKeyPairInput{ - KeyName: aws.String("inferred-rsa-created"), + KeyName: aws.String("ed25519-created"), + }, testAccountID) + require.NoError(t, err) + + _, err = svc.CreateKeyPair(context.Background(), &ec2.CreateKeyPairInput{ + KeyName: aws.String("rsa-created"), KeyType: aws.String("rsa"), }, testAccountID) require.NoError(t, err) @@ -600,12 +624,35 @@ func TestDescribeKeyPairs_KeyTypeInference(t *testing.T) { keyTypes[*kp.KeyName] = *kp.KeyType } assert.Equal(t, map[string]string{ - "inferred-ed25519": "ed25519", - "inferred-rsa-imported": "rsa", - "inferred-rsa-created": "rsa", + "ed25519-imported": "ed25519", + "ed25519-created": "ed25519", + "rsa-imported": "rsa", + "rsa-created": "rsa", }, keyTypes) } +// A record written before KeyType was persisted still has to describe as +// ed25519, and its fingerprint has to reach the client in the AWS spelling. This +// is the case the removal of the "SHA256:" prefix would otherwise have broken. +func TestDescribeKeyPairs_LegacyED25519Record(t *testing.T) { + svc, store := newTestKeyService() + + putStoredObject(t, store, "keys/"+testAccountID+"/key-legacy1.json", + `{"KeyFingerprint":"`+testLegacyED25519Fingerprint+`","KeyName":"legacy-ed25519","KeyPairId":"key-legacy1","Tags":null}`) + + out, err := svc.DescribeKeyPairs(context.Background(), &ec2.DescribeKeyPairsInput{}, testAccountID) + require.NoError(t, err) + require.Len(t, out.KeyPairs, 1) + + assert.Equal(t, "ed25519", *out.KeyPairs[0].KeyType) + assert.Equal(t, testED25519Fingerprint, *out.KeyPairs[0].KeyFingerprint) + + // Describing normalises the response only: a list must not become a write + // per key pair. + assert.Contains(t, string(readStoredObject(t, store, "keys/"+testAccountID+"/key-legacy1.json")), + testLegacyED25519Fingerprint) +} + func TestDescribeKeyPairs_FilterByKeyName(t *testing.T) { svc, _ := newTestKeyService() @@ -818,8 +865,7 @@ func TestImportedKeyFingerprint(t *testing.T) { pubKey string expected string }{ - // The OpenSSH rendering of the SHA256 digest, which is not what AWS - // returns -- AWS drops the prefix and pads the base64. + // The SHA256 of the SSH wire blob, in AWS's bare padded base64. {name: "ED25519", pubKey: testED25519PubKey, expected: testED25519Fingerprint}, // The MD5 of the DER SubjectPublicKeyInfo, matching AWS. {name: "RSA", pubKey: testRSAPubKey, expected: testRSAImportedFingerprint}, @@ -874,8 +920,8 @@ func TestCreatedKeyFingerprint_RSA(t *testing.T) { } // ED25519 hashes the public key on both paths, so the created value matches the -// imported one -- and diverges from AWS the same way. The private key is passed -// deliberately mismatched: this branch must not read it at all. +// imported one. The private key is passed deliberately mismatched: this branch +// must not read it at all. func TestCreatedKeyFingerprint_ED25519(t *testing.T) { fingerprint, err := createdKeyFingerprint([]byte(testRSAPrivKey), parseTestPubKey(t, testED25519PubKey)) require.NoError(t, err)