HDDS-11070. Separate KeyCodec from reading and storing keys to disk #6871
Conversation
fapifta
added
code-cleanup
There was a problem hiding this comment.
Thank you @Galsza for working on this one, I have a generic observation around changes like the one in RootCARotationManager, but there are other places as well, where we have a keypair, but instead of calling the storeKey method, we call the storePublicKey and the storePrivateKey method.
Is there a reason for using the sequence instead of the single method?
Besides this I left one more comment inline, other than these the patch looks good.
| KeyCodec pemWriter = new KeyCodec(securityConfig, component); | ||
| SecurityConfig secConfig = pemWriter.getSecurityConfig(); | ||
| pemWriter.writeKey(kp); | ||
| SecurityConfig secConfig = securityConfig; |
There was a problem hiding this comment.
Do we need this line here, at least it is strange, and if we anyways touch this code piece, let's fix this if it is not required, or if it is required for any reason, let's add a comment here on the why.
There was a problem hiding this comment.
I think the author was fixated on fitting lines in the old 80 character limit
Galsza
force-pushed
the
HDDS-11070
branch
2 times, most recently
from
3d0fdbd to
58c6885
Compare
|
This PR was abandoned for quite some time due to other refactoring regarding PKI. At first we didn't know if this PR would work looking at a larger picture, but now it seems that it will. More changes coming soon for PKI. Force push was used because this change was merged into a larger changeset and cherry picked back here after. |
|
Thanks @Galsza for updating the patch. Please check test failures in https://github.com/Galsza/ozone/actions/runs/12260851383/job/34207032724#step:6:1037 |
…er and handling persistence of keys
Purify KeyCodec and KeyStorage APIs, clear/add tests for the remaining pieces. Remove SecurityUtil, replace its functionality with the KeyCodec. KeyCodec now works with byte[] instead of String. KeyStorage relies purely on NIO. Keys related classes moved to hdds-common from hdds-framework. Intorduced SecurityConstants for string based magic constants in the PEM format. Changes in SecurityConfig to ensure testability. Updated APIDocs.
|
Please try to avoid force-push when updating the PR. Here are some great articles that explain why: https://developers.mattermost.com/blog/submitting-great-prs/#4-avoid-force-pushing Specifically, I had updated your branch from
|
|
@adoroszlai thank you for that and sorry for the force push, it was due to using a wrong commit that the second one was building on, as these commits are being cherry-picked from a separate fork. I won't force push like that the next time. Green CI is available here now: https://github.com/Galsza/ozone/actions/runs/12264912055 |
| } | ||
|
|
||
| private KeyStorage(SecurityConfig config, String component, Path keyPath) throws IOException { | ||
| if (config.useExternalCACertificate(component)) { |
There was a problem hiding this comment.
@Galsza , previous implementation, it reads the public and private key from external directory, then write and save them to Ozone's directory. In this new implementation, this rewrite is omitted. In this case, I would suggest check the permission of this external path too, make sure the path permission also comply with our requirement.
There was a problem hiding this comment.
@Galsza , thanks for updating the patch. What I means is we should check the external directory permission, and enforce some rule. If we cannot enforce "rwx------", at least we should enforce that others group cannot have any access to this directory, and group cannot write to this directory, for example "ozone.om.db.dirs.permissions". Because all the internal PKI system security is highly depends on the security of root private key. If anyone can access this external root certificate key materials, then the security will be more easy to compromise.
There was a problem hiding this comment.
@ChenSammi Thank you for the reply, I'm not sure how much we want to change the external permissions. Only the owner having access to it might be too restrictive, but we could give "rwxr-x---" (750). Ozone.om.db.dirs.permissions applies to the metadata directories, which might be less strict than what is required for the security. Is there a case where we don't want people specified in ozone.om.db.dirs.permissions to access security files? (I think yes, there are but I'm not familiar how these are used in production so I might be wrong here).
I can either specify a constant "rwxr-x---" or add a new property where we specify security permissions but that seems too much here. What do you think? @fapifta could you also take a look at this?
There was a problem hiding this comment.
We should file a separate jira for these permissions. The external private key should be set up in such a way that only the user running ozone can read it. The system could test if this is the case and either throw an error or log a warning. Preferably have a config option for deciding which happens
hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/security/x509/keys/TestKeyStorage.java
Outdated
Show resolved
Hide resolved
| this.privateKey = SecurityUtil.getPrivateKey(pvtKey, securityConfig); | ||
| this.publicKey = SecurityUtil.getPublicKey(publicKey, securityConfig); | ||
| } | ||
|
|
||
| public int getKeyId() { |
There was a problem hiding this comment.
Two readProtoBuf() methods calling this constructor look like not used anymore.
...n/java/org/apache/hadoop/hdds/security/x509/certificate/client/DefaultCertificateClient.java
Outdated
Show resolved
Hide resolved
| public void storePrivateKey(PrivateKey key) throws IOException { | ||
| LOG.info("Storing private key to {}.", privateKeyPath); | ||
| try { | ||
| storeKey(privateKeyPath, keyCodec.encodePrivateKey(key)); |
There was a problem hiding this comment.
The current implementation, it will prevent the private and public key from overwrite again if the file is already exists. I would prefer we keep this logic.
There was a problem hiding this comment.
The new implementation will also fail if we try overwriting the file. storeKey eventually runs into Files.create() method which throws a FileAlreadyExistsException.
KeyCodec is now only responsible for encoding and decoding keys, the rest of the functionality is in KeyStorage
For crypto-compliance we would like the non-compliant parts of the code to be as separated and as small as possible so that they will be easy to move to a new method. Here I'm separating these responsibilities so the KeyCodec is only encoding/decoding keys. Later it can be moved to a new module.
HDDS-11070
Updated, here is a fresh new CI run:
https://github.com/Galsza/ozone/actions/runs/12106597177