Add secure file setting to keystore #24001
Merged
Conversation
This file contains 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
Some systems like GCE rely on a plaintext file containing credentials. Rather than extract the information out of that credentials file and store each peace individually in the keystore, it is cleaner to just store the entire file. This commit adds support to the keystore wrapper for secure file settings. These are settings that contain an entire file that would normally be stored on the local filesystem. Retrieving the file returns an input stream to the file contents. This also adds a `add-file` command to the keystore cli. In order to support both strings and files as values for settings, the metadata format of the keystore has also been updated (with backcompat) to keep a map of setting name to type.
rjernst
added
:Core/Infra/Settings
s1monw
approved these changes
Apr 10, 2017
| String secretKeyAlgo = input.readString(); | ||
| String stringKeyAlgo = input.readString(); | ||
| final String fileKeyAlgo; | ||
| if (formatVersion >= 2) { |
There was a problem hiding this comment.
can we get a constant here instead of the plain number
There was a problem hiding this comment.
I'm not sure what that would help with. We have a constant for the min version supported, as well as the current version. I could create a constant for "v1" and "v2", but how is reading a constant name different than seeing the version directly here? It would just mean 2 is in the name, so not really different?
| Enumeration<String> aliases = keystore.get().aliases(); | ||
| while (aliases.hasMoreElements()) { | ||
| settingNames.add(aliases.nextElement()); | ||
| if (formatVersion == 1) { |
There was a problem hiding this comment.
same here please add a nice constant that is human readable
| // TODO: in the future if we ever change any algorithms used above, we need | ||
| // to create a new KeyStore here instead of using the existing one, so that | ||
| // the encoded material inside the keystore is updated | ||
| assert type.equals(NEW_KEYSTORE_TYPE); |
There was a problem hiding this comment.
please give us messages for the assertions
| KeyStore.SecretKeyEntry secretKeyEntry = (KeyStore.SecretKeyEntry) entry; | ||
| PBEKeySpec keySpec = (PBEKeySpec) fileFactory.getKeySpec(secretKeyEntry.getSecretKey(), PBEKeySpec.class); | ||
| // The PBE keyspec gives us chars, we first convert to bytes, then decode base64 inline. | ||
| char[] chars = keySpec.getPassword(); |
There was a problem hiding this comment.
so these are all 1 byte chars here in the password?
| @Override | ||
| public void close() throws IOException { | ||
| super.close(); | ||
| Arrays.fill(bytes, (byte)0); // wipe our second copy when the stream is exhausted |
| bytes = Base64.getEncoder().encode(bytes); | ||
| char[] chars = new char[bytes.length]; | ||
| for (int i = 0; i < chars.length; ++i) { | ||
| chars[i] = (char)bytes[i]; |
There was a problem hiding this comment.
again 1 byte chars? maybe leave a comment about this here and above?
There was a problem hiding this comment.
Yes, I already had a comment above. I copied it down to here as well:
// PBE only stores the lower 8 bits, so this narrowing is ok
jaymode
approved these changes
Apr 10, 2017
rjernst
added a commit
that referenced
this pull request
Apr 10, 2017
Some systems like GCE rely on a plaintext file containing credentials. Rather than extract the information out of that credentials file and store each peace individually in the keystore, it is cleaner to just store the entire file. This commit adds support to the keystore wrapper for secure file settings. These are settings that contain an entire file that would normally be stored on the local filesystem. Retrieving the file returns an input stream to the file contents. This also adds a `add-file` command to the keystore cli. In order to support both strings and files as values for settings, the metadata format of the keystore has also been updated (with backcompat) to keep a map of setting name to type.
clintongormley
changed the title
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.
Some systems like GCE rely on a plaintext file containing credentials.
Rather than extract the information out of that credentials file and
store each peace individually in the keystore, it is cleaner to just
store the entire file.
This commit adds support to the keystore wrapper for secure file
settings. These are settings that contain an entire file that would
normally be stored on the local filesystem. Retrieving the file returns
an input stream to the file contents. This also adds a
add-filecommand to the keystore cli.