Fix erroneous removal of secret prefixes in SecretSource #2007
Conversation
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
|
This is initially WIP, with a failing test that shows the problem. |
6 tasks
|
Digging deeper, the code flow inside // if var = 'arn:aws:secretsmanager:eu-central-1:123456789012:secret:my-secret'
public String lookup(String var) {
if (var == null) {
return null;
}
final int prefixPos = var.indexOf(PREFIX_SEPARATOR); // prefixPos = 3
if (prefixPos >= 0) {
final String prefix = toKey(var.substring(0, prefixPos)); // prefix = 'arn'
final String name = var.substring(prefixPos + 1); // name = 'aws:secretsmanager:eu-central-1:123456789012:secret:my-secret'
final StringLookup lookup = stringLookupMap.get(prefix); // lookup = null (because there is no CasC handler for 'arn' prefixes)
String value = null;
if (lookup != null) { // statement is skipped
value = lookup.lookup(name);
}
if (value != null) { // statement is skipped
return value;
}
var = var.substring(prefixPos + 1); // var = 'aws:secretsmanager:eu-central-1:123456789012:secret:my-secret'
}
if (defaultStringLookup != null) {
return defaultStringLookup.lookup(var); // ...and the SecretSource implementations are asked to find 'aws:secretsmanager:eu-central-1:123456789012:secret:my-secret'
}
return null;
} |
|
The bug is located here. Yes, we could write our own string lookup. var = var.substring(prefixPos + 1); // var = 'aws:secretsmanager:eu-central-1:123456789012:secret:my-secret' |
|
Do you think it's worth reporting this as a bug upstream in |
|
(And potentially filing a PR to fix this upstream?) |
|
If the maintainer says it is not a bug at least a fix could also be to have lookup option to preserve prefix or remove prefix. Though I honestly think this is a bug. This should not remove the prefix! // if var = 'arn:aws:secretsmanager:eu-central-1:123456789012:secret:my-secret', preservePrefix = true
public String lookup(String var, boolean preservePrefix) {
if (var == null) {
return null;
}
final int prefixPos = var.indexOf(PREFIX_SEPARATOR); // prefixPos = 3
if (prefixPos >= 0) {
final String prefix = toKey(var.substring(0, prefixPos)); // prefix = 'arn'
final String name = var.substring(prefixPos + 1); // name = 'aws:secretsmanager:eu-central-1:123456789012:secret:my-secret'
final StringLookup lookup = stringLookupMap.get(prefix); // lookup = null (because there is no CasC handler for 'arn' prefixes)
String value = null;
if (lookup != null) { // statement is skipped
value = lookup.lookup(name);
}
if (value != null) { // statement is skipped
return value;
}
if (preservePrefix == false) {
var = var.substring(prefixPos + 1); // var = 'aws:secretsmanager:eu-central-1:123456789012:secret:my-secret'
}
}
if (defaultStringLookup != null) {
return defaultStringLookup.lookup(var); // ...and the SecretSource implementations are asked to find 'arn:aws:secretsmanager:eu-central-1:123456789012:secret:my-secret'
}
return null;
} |
|
@chriskilding I decided to implement a fix to unblock this 😅 |
|
We have decent test coverage that I feel comfortable with this change. @timja @oleg-nenashev WDYT |
Codecov Report
@@ Coverage Diff @@
## master #2007 +/- ##
============================================
- Coverage 80.89% 80.86% -0.03%
- Complexity 837 844 +7
============================================
Files 71 72 +1
Lines 2465 2488 +23
Branches 346 351 +5
============================================
+ Hits 1994 2012 +18
- Misses 362 365 +3
- Partials 109 111 +2
|
timja
approved these changes
Jun 27, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Only remove secret prefixes when they correspond to one of the SecretSource protocol substitutors (e.g.
base64:,file:). If a secret prefix is not handled by the substitutors, leave it alone.fixes #1949
Your checklist for this pull request
🚨 Please review the guidelines for contributing to this repository.