fix: stop misclassifying plain S3 credentials as base64 (#2245) - #955
Open
PutlaTheophila wants to merge 3 commits into
Open
fix: stop misclassifying plain S3 credentials as base64 (#2245)#955PutlaTheophila wants to merge 3 commits into
PutlaTheophila wants to merge 3 commits into
Conversation
The DataImportJob mutating webhook previously used the
utils.IsBase64Encoded heuristic to decide whether to store the inline
AccessKeyID / SecretAccessKey into the credentials Secret as raw bytes
(Data, treated as already-base64-encoded by the API server) or as plain
strings (StringData). The heuristic is
len(s)%4 == 0 && len(s) > 0 && err == nil
where err comes from base64.StdEncoding.DecodeString. Any plain ASCII
string whose length is a multiple of 4 with no internal "=" passes
DecodeString silently, so a real AWS access key like
AKIAIOSFODNN7EXAMPLEAAAAAAAAAAAA (length 32)
is misclassified as base64 and stored as the decoded byte sequence.
The DataImport job then receives a corrupted AWS_ACCESS_KEY_ID env
var and the import either fails with "InvalidAccessKeyId" or hangs in
"importing" state, with no surface-level error to the user.
Drop the heuristic in mutateS3CredentialsSecret and always write the
user-supplied credentials to secret.StringData. The API server encodes
StringData on the write path, so plain credentials round-trip unchanged.
Tests:
- TestDataImportJobDefaulter now uses 32-char plain-alphanumeric keys
(the regression class) and asserts the values land in StringData
verbatim.
- TestDatabaseClusterDefaulter updated for the same assertion since it
exercises the same handleS3CredentialsSecret path through the
DatabaseCluster defaulter.
- TestIsBase64Encoded is removed since the function is no longer used
here (it remains in utils for SplitHorizonDNSConfig cert validation,
which is out of scope for this fix).
The sibling openeverest PR adds a synchronous read-only S3 probe at
DatabaseCluster admission so the broader "credentials silently fail"
class of bug (openeverest/openeverest#2229) is caught up front.
Fixes openeverest/openeverest#2245
Signed-off-by: PutlaTheophila <putlatheophila123@gmail.com>
4 tasks
chilagrow
reviewed
Jun 3, 2026
chilagrow
left a comment
chilagrow
left a comment
Contributor
There was a problem hiding this comment.
Thanks for your contribution! One tiny comment.
Comment on lines
+81
to
+82
| // Credentials land in StringData verbatim. Writing to Data with these | ||
| // values would have stored a corrupted (base64-decoded) byte sequence. |
Contributor
There was a problem hiding this comment.
Let's remove this comment, for the readers of the code this comment sounds confusing without context.
PutlaTheophila
added a commit
to PutlaTheophila/openeverest-operator
that referenced
this pull request
Jun 3, 2026
Addresses review feedback from @chilagrow on PR openeverest#955.
Author
|
Thanks for the review @chilagrow! Removed the confusing |
chilagrow
reviewed
Jun 3, 2026
chilagrow
left a comment
chilagrow
left a comment
Contributor
There was a problem hiding this comment.
Could you sign-off your commit please? CI is failing
Addresses review feedback from @chilagrow on PR openeverest#955. Signed-off-by: PutlaTheophila <putlatheophila123@gmail.com>
…4-heuristic Signed-off-by: PutlaTheophila <putlatheophila123@gmail.com>
PutlaTheophila
force-pushed
the
fix/2245-drop-base64-heuristic
branch
from
June 3, 2026 12:42
c0395d0 to
4e67ec5
Compare
Author
|
Thanks @chilagrow — both addressed:
|
chilagrow
reviewed
Jun 4, 2026
chilagrow
left a comment
chilagrow
left a comment
Contributor
There was a problem hiding this comment.
Thanks for your contribution! I run e2e test for data importer a few times and they are failing, could you look at it please https://github.com/openeverest/openeverest-operator/actions/runs/26921958578/job/79426452666?pr=955?
Note, we run these tests manually upon change of the relevant code, so it might have got outdated 🙏
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The DataImportJob mutating webhook used
utils.IsBase64Encodedto decide whether to store inlineAccessKeyID/SecretAccessKeyinto the credentials Secret asData(raw bytes, treated by the API server as already-base64) or asStringData(plain text). The heuristic isbase64.StdEncoding.DecodeStringsilently accepts any ASCII string whose length is a multiple of 4 with no internal=. Real AWS access keys likeare therefore misclassified as base64 and stored as the decoded byte sequence. The DataImport job then receives a corrupted
AWS_ACCESS_KEY_IDenv var, S3 returnsInvalidAccessKeyId, and the DB sits inimportingwith no surface-level error.Fix
mutateS3CredentialsSecret. Always write user-supplied credentials tosecret.StringData— the API server encodes them on the write path, so plain strings round-trip unchanged.utils.IsBase64Encodedis retained becauseinternal/webhook/enginefeatures.everest/v1alpha1/splithorizondnsconfig_webhook.gostill calls it for CA cert/key validation. Changing that webhook's contract is out of scope here.Tests
TestDataImportJobDefaulternow uses 32-char plain-alphanumeric keys (the regression class) and asserts the values land inStringDataverbatim.TestDatabaseClusterDefaulterupdated for the same assertion — it exercises the samehandleS3CredentialsSecretpath through theDatabaseClusterdefaulter.TestIsBase64Encodedremoved (the function is no longer called from this package).Related
Companion PR in the API server adds a synchronous read-only S3 probe at
DatabaseClusteradmission so the broader "credentials silently fail" class of bug is caught at create time: openeverest/openeverest#2279 (Fixes openeverest/openeverest#2229).Together the two PRs make the inline-credentials path correct end-to-end: the operator stores them as plain strings, and the API server validates them against S3 before the resource is admitted.
Test plan
go build ./...cleango vet ./internal/webhook/everest/v1alpha1/... ./utils/... ./internal/webhook/enginefeatures.everest/v1alpha1/...cleango test ./internal/webhook/everest/v1alpha1/... ./utils/... ./internal/webhook/enginefeatures.everest/v1alpha1/...passFixes openeverest/openeverest#2245.