Secure key storage #481

MatheusFranco99 · 2024-08-14T10:49:06Z

Overview

This PR adjusts the PemToPrivateKey and PemToPublicKey functions to use a secure storage method, removing the deprecated functions that were being used.

GalRogozinski · 2024-08-15T07:12:24Z

types/encryption.go

@@ -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)


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

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

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

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

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

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.

y0sher · 2024-08-19T09:06:36Z

types/encryption.go

-	parsedSk, err := x509.ParsePKCS1PrivateKey(b)
+
+	// Parse key as PKCS8
+	parsedSk, err := x509.ParsePKCS8PrivateKey(block.Bytes)


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

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

y0sher

@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.

MatheusFranco99 · 2024-08-19T11:41:49Z

@y0sher
Sure
Adding back the PKCS1 due to the break issues

y0sher · 2024-08-19T12:48:49Z

@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.

MatheusFranco99 · 2024-08-19T13:29:40Z

@y0sher Agree 100%. Thanks

alan-ssvlabs

Agree with the discussions above

GalRogozinski · 2024-09-01T13:53:50Z

@y0sher
there is no hinderance now that we switched back to PKCS1 and this can be merged from your perspective?

y0sher · 2024-09-02T09:53:33Z

Other than that looks good to me.

GalRogozinski · 2024-09-02T10:01:22Z

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.
I understood the tags will be helpful if we ever change our encryption algo, when we do that the encoding can change.

* Implement secure key storage using PKCS8 * Fix lint issue * Switch back PKCS8 to PKCS1

* remove domaintype from committeeMember * remove domaintype from share * add config for ssv containing domainType * add custom unmarshal for baserunner to avoid error in unmarshalling config * move domainType to Network * add testing domain in NewTestingNetwork function * Spec - filter committee duties per slot validator duties (#467) * Filter shares for slot `CommitteeRunner` based on validators that have duty for that slot. * Filter duty and create share map according to owned validators * Add test: start duty with no shares for duty's validators * Add test: happy flow for committee with fraction of duty's validators * Generate JSON tests * Apply suggestions --------- Co-authored-by: MatheusFranco99 <48058141+MatheusFranco99@users.noreply.github.com> * Spec - check att and sync duties exist before submitting (#468) * Meta - Update to go1.22 (#474) * Update go1.20 to go1.22 * Update go.sum with mod tidy * Meta - Update dependencies (#483) * Update dependencies * Fix lint issue * Generate JSON tests to trigger actions * Update fastssz * Generate JSON tests and align ssz error * Revert go-eth2-client version change * Revert fastssz upgrade * Generate SSZ and JSON tests * Meta - Fix static analysis issues (#480) * Solve potential file inclusion via variable * Fix file permission (0644 to 0600) * Add nosec comment for PRNG (pseudo-random number generator) used for testing * Fix lint issue on nil check in []byte type * Update permission from 0444 to 0600 * Update 0444 to 0400 * Meta - Drop unnecessary nolint comments (#477) * Remove nolint comment and export timeout variables * Drop unnecessary nolint * Add comment * Fix lint issue * Spec - Share length validation (#478) * Add share length validation in runner construction * Align to error handling in runners constructions * Add validation to committee runner * Add runner construction tests * Refactor runner construction in testingutil to deal with creation errors * Generate JSON tests * Fix lint issue * Fix comments * Meta - Drop redundant error (#475) * Spec - Drop redundant validation for decided messages (#476) * Remove redundant validation * Align error string * Spec - Sort signers in decided message (#484) * Sort signers in decided message * Add test for sorted signers in decided msg * Generate JSON tests * Fix lint issue * Spec - Stop processing after decided (#487) * Stop processing consensus messages after instance is decided * Align error in qbft tests * Align errors in ssv tests * Generate JSON tests * Fix lint issue * Spec - Drop leftover error check (#469) * Remove leftover err check * Align argument variable name to type * Spec - Secure key storage (#481) * Implement secure key storage using PKCS8 * Fix lint issue * Switch back PKCS8 to PKCS1 * Meta - Remove residual DKG (#502) * Remove DKG signature type * Remove DKG msg type * Remove DKGOperators field from TestKeySet * Remove unused ecdsaKeys field from TestingKeyStorage * Remove unused "ecdsaSKFromHex" function * Generate JSON tests * Spec - Add GoSec action and fix issues (#505) * Add github action and makefile command * Fix issues in round robin proposer function * Fix bad PutUint32 in GetCommitteeID * Fix issue with HasQuorum and HasPartialQuorum * Add role sanitization in GetRoleType and NewMessageType * Add sanitization to BeaconNetwork methods * Add sanitization in testingutils * Add sanitization to height usage in test files * Fix uint64 conversion in runner/postconsensus/valid_msg test * Sanitize ValidatorIndex conversion * Update action name * Fix tests to use valid RunnerRoles * Generate SSZ * Generate JSON tests * Revert the change on GetCommitteeID * Add nosec G115 to GetCommitteeID * revert the merge because it was merged with origin main by accident --------- Co-authored-by: rehs0y <lyosher@gmail.com> Co-authored-by: MatheusFranco99 <48058141+MatheusFranco99@users.noreply.github.com>

MatheusFranco99 added 2 commits August 14, 2024 11:46

Implement secure key storage using PKCS8

7c1bf17

Fix lint issue

205cf62

MatheusFranco99 requested a review from GalRogozinski August 14, 2024 10:49

MatheusFranco99 self-assigned this Aug 14, 2024

GalRogozinski reviewed Aug 15, 2024

View reviewed changes

GalRogozinski added the spec label Aug 15, 2024

GalRogozinski approved these changes Aug 15, 2024

View reviewed changes

GalRogozinski mentioned this pull request Aug 15, 2024

A spec probably shouldn't care about whether the key is PKCS1 or PKCS8 encoded #485

Closed

MatheusFranco99 requested a review from alan-ssvlabs August 16, 2024 08:21

y0sher reviewed Aug 19, 2024

View reviewed changes

y0sher approved these changes Aug 19, 2024

View reviewed changes

Switch back PKCS8 to PKCS1

ef19a85

MatheusFranco99 mentioned this pull request Aug 19, 2024

Update PKCS1 to PKCS8 #489

Open

alan-ssvlabs approved these changes Aug 19, 2024

View reviewed changes

GalRogozinski merged commit 6342e32 into ssvlabs:dev Sep 2, 2024
2 checks passed

GalRogozinski pushed a commit that referenced this pull request Sep 2, 2024

Spec - Secure key storage (#481)

5b5e3dc

* Implement secure key storage using PKCS8 * Fix lint issue * Switch back PKCS8 to PKCS1

GalRogozinski pushed a commit that referenced this pull request Sep 12, 2024

Spec - Secure key storage (#481)

d502f36

* Implement secure key storage using PKCS8 * Fix lint issue * Switch back PKCS8 to PKCS1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Secure key storage #481

Secure key storage #481

MatheusFranco99 commented Aug 14, 2024

GalRogozinski Aug 15, 2024

MatheusFranco99 Aug 15, 2024 •

edited

Loading

GalRogozinski Aug 15, 2024

GalRogozinski Aug 15, 2024

MatheusFranco99 Aug 15, 2024 •

edited

Loading

y0sher Aug 19, 2024

y0sher Aug 19, 2024

MatheusFranco99 Aug 19, 2024

y0sher left a comment

MatheusFranco99 commented Aug 19, 2024

y0sher commented Aug 19, 2024

MatheusFranco99 commented Aug 19, 2024

alan-ssvlabs left a comment

GalRogozinski commented Sep 1, 2024

y0sher commented Sep 2, 2024

GalRogozinski commented Sep 2, 2024

Secure key storage #481

Secure key storage #481

Conversation

MatheusFranco99 commented Aug 14, 2024

Overview

GalRogozinski Aug 15, 2024

Choose a reason for hiding this comment

MatheusFranco99 Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

GalRogozinski Aug 15, 2024

Choose a reason for hiding this comment

GalRogozinski Aug 15, 2024

Choose a reason for hiding this comment

MatheusFranco99 Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

y0sher Aug 19, 2024

Choose a reason for hiding this comment

y0sher Aug 19, 2024

Choose a reason for hiding this comment

MatheusFranco99 Aug 19, 2024

Choose a reason for hiding this comment

y0sher left a comment

Choose a reason for hiding this comment

MatheusFranco99 commented Aug 19, 2024

y0sher commented Aug 19, 2024

MatheusFranco99 commented Aug 19, 2024

alan-ssvlabs left a comment

Choose a reason for hiding this comment

GalRogozinski commented Sep 1, 2024

y0sher commented Sep 2, 2024

GalRogozinski commented Sep 2, 2024

MatheusFranco99 Aug 15, 2024 •

edited

Loading

MatheusFranco99 Aug 15, 2024 •

edited

Loading