TLS Auth - Support for encrypted private key #2488
Conversation
|
|
Gabrielopesantos
force-pushed
the
feat/support-for-encrypted-keys-in-tlsauth
branch
from
de624f1 to
2ea9573
Compare
lib/options.go
Outdated
| return nil, fmt.Errorf("failed to decode PEM key") | ||
| } | ||
| blockType := block.Type | ||
| if encrypted := x509.IsEncryptedPEMBlock(block); encrypted { |
There was a problem hiding this comment.
If we have password, shouldn't we always try to decrypt it?
There was a problem hiding this comment.
If we always try decrypt, in cases were a non encrypted key is provided, since it does not a DEK header, x509.DecryptPEMBlock (https://pkg.go.dev/crypto/x509#DecryptPEMBlock) will return an error. In my opinion a good approach would be just add the warning saying that a password is not required if the key is not encrypted, as suggested below.
If both of you you agree with this, if possible, tell me how can I add a warning.
There was a problem hiding this comment.
I guess that you can't log here 🤔 . But on the other hand I would argue it should be an outright error if there is a password, and it isn't encrypted. If you have a password and not DEK header ... you maybe shouldn't have password ?
There was a problem hiding this comment.
Yes, I understand your point. I just thought it would confuse some users the first time they use the feature. Made the change.
There was a problem hiding this comment.
confuse some users
I would argue it will be even more confusing if you add an option and nothing changes - if we at least then error out differently IMO it's better.
Codecov Report
@@ Coverage Diff @@
## master #2488 +/- ##
==========================================
+ Coverage 72.71% 75.44% +2.72%
==========================================
Files 184 200 +16
Lines 14571 15877 +1306
==========================================
+ Hits 10596 11978 +1382
+ Misses 3333 3164 -169
- Partials 642 735 +93
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
It seems like the tests aren't passing after the last changes @Gabrielopesantos, can you take a look 🙇 . I will try to test them by hand by the end of the week and hopefully we can have this merge next week 🤞 |
|
Ok,updated the tests and also added a new tests covering the case where a user defines a password with a non encrypted. The CI seems to still be failing, maybe adding |
|
Yeah - you need will need to |
There was a problem hiding this comment.
Thanks a lot for working on this 🙇
LGTM - I have tried it out locally, and I don't see any problems, although I can't really make requests with it as I don't have any server that uses client certificates handy :(. That shouldn't matter though as that is handled by the stdlib - we just need to give it the certificate.
I have left one change on a comment, but I am not even certain it is all that much better than before, so we might leave it.
Co-authored-by: Mihail Stoykov <[email protected]>
Related issue: #2435 (Also presents the problem)
Proposal: