Validate key expiration time #22
Conversation
slawekjaranowski
force-pushed
the
fix-7
branch
from
481beda
to
7fee01d
Compare
|
Kudos, SonarCloud Quality Gate passed! |
| */ | ||
| public static void verifyKeyExpiration(PGPSecretKey secretKey, PGPSecretKeyRing secretKeyRing) { | ||
|
|
||
| long validSeconds = secretKey.getPublicKey().getValidSeconds(); |
There was a problem hiding this comment.
I wonder if that is always correct. There is a method PGPSecretKey.replacePublicKey that would set another public key with a different valid seconds number into the same secret key. Effectively our code here would see the new expiration date. Is that correct?
There was a problem hiding this comment.
I don't know what for is method PGPSecretKey.replacePublicKey but in our code it is not call. We read secret key from file and don't change content of it. So it will be always correct.
There was a problem hiding this comment.
My fear actually is not our code calling that on its own, but whether that other external tools modify the key file in a way that replacePublicKey does, and I wonder what the outcome is: Will our code still see the expiration date of the secret key, or will it now see the expiration date of that new public key? As this question has impact on the correctness of our validation check, maybe we should ask at the Bouncy Castle user list to be on the safe side?
There was a problem hiding this comment.
We can try to verify internal key signature, each uid and subkey has self signed signature, but we are in the same situation if somebody can change file with key probably generate new valid self signature.
Another idea is verify system access level to key file - if only is writable / readable by owner.
I will assume that key file is protected correctly by operating system.
There was a problem hiding this comment.
Before hastily hacking in something that we possibly don't need at all, I have asked our questions at Bouncy Castle. If you like, we can merge the PR as is and I will file the open questions as an open issue in our tracker?
There was a problem hiding this comment.
ok, we can open issue on our list and link with bc
src/main/java/org/simplify4u/plugins/sign/openpgp/PGPSecretKeyUtils.java
Show resolved
Hide resolved
There was a problem hiding this comment.
I am generally +1 for the benefits this PR brings, but actually vote 0 because I am not convinced that we actually read the corrrect date out of the key as I mentioned in my comment. Feel free to merge if you like, but I just have a bad gut feeling about this question being uncleared.
fix #7