Secure key storage #481
Secure key storage #481
Conversation
types/encryption.go
Outdated
| @@ -92,13 +88,17 @@ func PemToPublicKey(pkPem []byte) (*rsa.PublicKey, error) { | |||
| } | |||
|
|
|||
| // PrivateKeyToPem converts privateKey to pem encoded | |||
| func PrivateKeyToPem(sk *rsa.PrivateKey) []byte { | |||
| func PrivateKeyToPem(sk *rsa.PrivateKey) ([]byte, error) { | |||
| pemBytes, err := x509.MarshalPKCS8PrivateKey(sk) | |||
There was a problem hiding this comment.
I see in SSV they use x509.MarshalPKCS1PrivateKey(sk)
Why the difference?
This function isn't deprectated?
There was a problem hiding this comment.
From a discussion with Alan and as far as I understood, PKCS8 will just add a key-type tag. Follow this link for more
So, it's more or less the same but it seems that PKCS8 is preferable if we can use it (the audit also suggests it). Therefore, it would be good if SSV also updates it. If not, I can restore to PKCS1
There was a problem hiding this comment.
The only thing is maybe node stores private keys and if we switch formats it may cause issues...
I can give an approve for now... but we should wait for @moshe-blox or @y0sher to read this comment and give their opinion
There was a problem hiding this comment.
Btw, to be honest... this is an example of where we have too much implementation details in spec...
A spec probably shouldn't care about whether the key is PKCS1 or PKCS8 encoded
There was a problem hiding this comment.
Yup, I agree
The best would be either to have another repo to deal with this stuff (and spec could call its functions if necessary) or to define a function type and leave the node to implement it
There was a problem hiding this comment.
as commented above AFAIK, PKCS8 isn't backward compatible with PKCS1, so if we decide to go forward with this we'll need to do some migration in the node. The main issue is that private keys are created and stored outside of the node by operators. and they'll have to change the way their keys are encoded if we stop supporting PKCS1.
types/encryption.go
Outdated
| parsedSk, err := x509.ParsePKCS1PrivateKey(b) | ||
|
|
||
| // Parse key as PKCS8 | ||
| parsedSk, err := x509.ParsePKCS8PrivateKey(block.Bytes) |
There was a problem hiding this comment.
AFAIK PKCS8 isn't backward compatible with PKCS1.. wouldn't this break stuff?
There was a problem hiding this comment.
Correct
It depends on how these functions are being used. From your above comment, it seems that it would break indeed. So, I'll switch back to PKCS1
There was a problem hiding this comment.
@GalRogozinski @MatheusFranco99
(@moshe-blox, @lior-blox ).
I think I can approve this for the spec as I understand why you want this change. But I can't tell if and when we'll be able to adapt in the node. So maybe its best to wait with merging this.
|
@y0sher |
|
@MatheusFranco99 I would create an issue about switching to PKCS8 in the future if its better in any terms, we just need to to this carefully and think about it from a product perspective. |
|
@y0sher Agree 100%. Thanks |
|
@y0sher |
|
Other than that looks good to me. |
|
Btw @MatheusFranco99 and @alan-ssvlabs @y0sher, I don't see a reason to open an issue with PKCS8 unless someone can tell me a clear advantage of it that is relevant to us. |
* Implement secure key storage using PKCS8 * Fix lint issue * Switch back PKCS8 to PKCS1
* Implement secure key storage using PKCS8 * Fix lint issue * Switch back PKCS8 to PKCS1
Overview
This PR adjusts the
PemToPrivateKeyandPemToPublicKeyfunctions to use a secure storage method, removing the deprecated functions that were being used.