Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Secure key storage #481

Merged
merged 3 commits into from
Sep 2, 2024
Merged

Secure key storage #481

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7c1bf17
Implement secure key storage using PKCS8
MatheusFranco99 Aug 14, 2024
205cf62
Fix lint issue
MatheusFranco99 Aug 14, 2024
ef19a85
Switch back PKCS8 to PKCS1
MatheusFranco99 Aug 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ssv/runner_validations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ func (b *BaseRunner) ValidatePostConsensusMsg(runner Runner, psigMsgs *types.Par
}

// TODO https://github.com/ssvlabs/ssv-spec/issues/142 need to fix with this issue solution instead.
if b.State.DecidedValue == nil || len(b.State.DecidedValue) == 0 {
if len(b.State.DecidedValue) == 0 {
return errors.New("no decided value")
}

Expand Down
54 changes: 27 additions & 27 deletions types/encryption.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,10 @@ func GenerateKey() ([]byte, []byte, error) {
}

// convert to bytes
skPem := PrivateKeyToPem(sk)
skPem, err := PrivateKeyToPem(sk)
if err != nil {
return nil, nil, errors.Wrap(err, "Failed to encode private key to pem")
}
pkPem, err := GetPublicKeyPem(sk)
if err != nil {
return nil, nil, errors.Wrap(err, "Failed to marshal public key")
Expand Down Expand Up @@ -49,39 +52,32 @@ func Encrypt(pk *rsa.PublicKey, plainText []byte) ([]byte, error) {
// PemToPrivateKey return rsa private key from pem
func PemToPrivateKey(skPem []byte) (*rsa.PrivateKey, error) {
block, _ := pem.Decode(skPem)
// nolint
enc := x509.IsEncryptedPEMBlock(block)
b := block.Bytes
if enc {
var err error
// nolint
b, err = x509.DecryptPEMBlock(block, nil)
if err != nil {
return nil, errors.Wrap(err, "Failed to decrypt private key")
}
if block == nil {
return nil, errors.New("failed to decode PEM")
}
parsedSk, err := x509.ParsePKCS1PrivateKey(b)

// Parse key as PKCS8
parsedSk, err := x509.ParsePKCS8PrivateKey(block.Bytes)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK PKCS8 isn't backward compatible with PKCS1.. wouldn't this break stuff?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if err != nil {
return nil, errors.Wrap(err, "Failed to parse private key")
}
return parsedSk, nil

// Type assert to *rsa.PrivateKey
ret, ok := parsedSk.(*rsa.PrivateKey)
if !ok {
return nil, errors.New("Failed to parse private key")
}

return ret, nil
}

// PemToPublicKey return rsa public key from pem
func PemToPublicKey(pkPem []byte) (*rsa.PublicKey, error) {
block, _ := pem.Decode(pkPem)
// nolint
enc := x509.IsEncryptedPEMBlock(block)
b := block.Bytes
if enc {
var err error
// nolint
b, err = x509.DecryptPEMBlock(block, nil)
if err != nil {
return nil, errors.Wrap(err, "Failed to decrypt private key")
}
if block == nil {
return nil, errors.New("failed to decode PEM")
}
parsedPk, err := x509.ParsePKIXPublicKey(b)
parsedPk, err := x509.ParsePKIXPublicKey(block.Bytes)
if err != nil {
return nil, errors.Wrap(err, "Failed to parse public key")
}
Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see in SSV they use x509.MarshalPKCS1PrivateKey(sk)
Why the difference?
This function isn't deprectated?

Copy link
Contributor Author

@MatheusFranco99 MatheusFranco99 Aug 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@MatheusFranco99 MatheusFranco99 Aug 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if err != nil {
return nil, err
}
return pem.EncodeToMemory(
&pem.Block{
Type: "RSA PRIVATE KEY",
Bytes: x509.MarshalPKCS1PrivateKey(sk),
Bytes: pemBytes,
},
)
), nil
}

// GetPublicKeyPem get public key from private key and return []byte represent the public key
Expand Down