PSS RSA support in darwin and windows #18

avivklas · 2020-09-01T12:28:40Z

fixes #17

piax93 · 2020-11-05T10:56:21Z

This is some cool work, +1 on getting this reviewed and merged please (@btoews)

btoews · 2020-11-05T15:27:23Z

I'm not at GitHub any more. @ptoomey3 might be able to help.

ptoomey3 · 2020-11-05T15:39:01Z

/cc @github/prodsec for triage.

vcsjones

Hello! Thanks for the pull request, we'd love to be able to accept this PR after some questions are answered and some feedback is considered.

Aside from the inline comments and suggestions, can you please add some tests for this in certstore_tests.go?

certstore_darwin.go

vcsjones · 2020-11-17T20:03:36Z

certstore_windows.go

@@ -441,7 +457,8 @@ func (wpk *winPrivateKey) cngSignHash(hash crypto.Hash, digest []byte) ([]byte,
 }

 // capiSignHash signs a digest using the CryptoAPI APIs.
-func (wpk *winPrivateKey) capiSignHash(hash crypto.Hash, digest []byte) ([]byte, error) {
+func (wpk *winPrivateKey) capiSignHash(opts crypto.SignerOpts, digest []byte) ([]byte, error) {
+	hash := opts.HashFunc()


Since CAPI does not support the RSA-PSS algorithm, would it make sense to check if capiSignHash was given an opts that successfully casts to rsa.PSSOptions and return an error?

I see what you're saying, but will the PSS flow ever get to this func?

Probably not, but I think it would add some clarity that PSS is never expected to work with CAPI, in case someone does manage to get in to this path. I'd rather a clear "not supported" error than a strange Win32 error or something not working.

vcsjones · 2020-11-17T20:11:51Z

certstore_windows.go

+	enc := make([]byte, 2, 10)
+	copy(enc, "0x")
+	h := strconv.AppendUint(enc, uint64(ss), 16)
+	return fmt.Sprintf("SECURITY_STATUS %s", string(h))


I think something like this would be more idiomatic:

Suggested change

enc := make([]byte, 2, 10)

copy(enc, "0x")

h := strconv.AppendUint(enc, uint64(ss), 16)

return fmt.Sprintf("SECURITY_STATUS %s", string(h))

return fmt.Sprintf("SECURITY_STATUS 0x%08X", ss)

vcsjones · 2020-11-17T20:36:14Z

certstore_windows.go

+
+		if pssOpts, ok := opts.(*rsa.PSSOptions); ok {
+			saltLen := pssOpts.SaltLength
+			if saltLen < 0 {


SaltLength has two magic values, PSSSaltLengthAuto is 0 for an auto value, and PSSSaltLengthEqualsHash is -1.

We should not set the saltLen to len(digest) for anything other than PSSSaltLengthEqualsHash/-1 (maybe a new magic value will be introduced in the future).

I don't believe CNG will accept a value of "0" to mean "auto" the same way that it means in other places. The documentation states that auto is:

PSSSaltLengthAuto causes the salt in a PSS signature to be as large as possible when signing, and to be auto-detected when verifying.

Can we replicate that auto length behavior if the value is 0?

sure. nice catch :)

avivklas

good points! applied most of them but I'd like to deepen the discussion about one of them. please tell me what you think :)

certstore_darwin.go

avivklas · 2021-01-20T20:36:32Z

certstore_windows.go

+
+		if pssOpts, ok := opts.(*rsa.PSSOptions); ok {
+			saltLen := pssOpts.SaltLength
+			if saltLen < 0 {


sure. nice catch :)

avivklas · 2021-01-20T20:37:44Z

certstore_windows.go

@@ -441,7 +457,8 @@ func (wpk *winPrivateKey) cngSignHash(hash crypto.Hash, digest []byte) ([]byte,
 }

 // capiSignHash signs a digest using the CryptoAPI APIs.
-func (wpk *winPrivateKey) capiSignHash(hash crypto.Hash, digest []byte) ([]byte, error) {
+func (wpk *winPrivateKey) capiSignHash(opts crypto.SignerOpts, digest []byte) ([]byte, error) {
+	hash := opts.HashFunc()


I see what you're saying, but will the PSS flow ever get to this func?

avivklas · 2021-02-21T06:52:40Z

@vcsjones, sorry for this nudge, just making sure you haven't missed my changes/comments 😊

vcsjones

The changes look good, but I think it would still make sense to add an explicit error in the PSS-CAPI path. Also, I think we need some tests around this to make sure things are working (and continue to).

vcsjones · 2021-08-26T15:04:11Z

certstore_windows.go

@@ -441,7 +457,8 @@ func (wpk *winPrivateKey) cngSignHash(hash crypto.Hash, digest []byte) ([]byte,
 }

 // capiSignHash signs a digest using the CryptoAPI APIs.
-func (wpk *winPrivateKey) capiSignHash(hash crypto.Hash, digest []byte) ([]byte, error) {
+func (wpk *winPrivateKey) capiSignHash(opts crypto.SignerOpts, digest []byte) ([]byte, error) {
+	hash := opts.HashFunc()


Probably not, but I think it would add some clarity that PSS is never expected to work with CAPI, in case someone does manage to get in to this path. I'd rather a clear "not supported" error than a strange Win32 error or something not working.

lgarron · 2021-10-04T20:23:11Z

This project is now part of smimesign.
Please send this PR to https://github.com/github/smimesign if it's still relevant!

avivklas added 5 commits September 1, 2020 15:23

#17 fix wrong PSS RSA handling

e158315

added the ability to choose store location

2a1be9d

Merge branch 'store-location' into main

1f34ce5

added the ability to choose store location

5d18983

added the ability to choose store location

ece7f1d

ptoomey3 requested a review from a team November 5, 2020 15:38

vcsjones reviewed Nov 17, 2020

View reviewed changes

avivklas added 2 commits January 20, 2021 22:43

#17 made signing in darwin use PSS only for rsa

38ac2d3

#17 code improvements

9170544

avivklas commented Jan 20, 2021

View reviewed changes

avivklas added 3 commits January 20, 2021 22:57

#17 fixed missing cast for windows

824f68a

#17 removed unused import

721977d

#17 fixed error string building in windows

20503ac

avivklas requested a review from vcsjones January 20, 2021 21:55

mbyczkowski mentioned this pull request Apr 23, 2021

Keychain support fails on go 1.15 ghostunnel/ghostunnel#335

Closed

csstaub mentioned this pull request Apr 24, 2021

Support RSA-PSS on macOS (for TLS 1.3+) #25

Closed

sailorfrag mentioned this pull request May 28, 2021

Unfork certstore tailscale/tailscale#2005

Closed

csstaub added a commit to ghostunnel/ghostunnel that referenced this pull request Jun 8, 2021

Merge RSA-PSS support for Windows from github/certstore#18

23108a6

vcsjones suggested changes Aug 26, 2021

View reviewed changes

avivklas added 3 commits August 31, 2021 21:53

Merge remote-tracking branch 'origin/main' into pss-support

745d1e7

#17 better error clarity for irrational capi flow with

f1cc86d

#17 added tests with PSS signOpts

f4f96f7

avivklas requested a review from vcsjones August 31, 2021 21:30

avivklas added 2 commits September 2, 2021 08:36

#17 better error clarity for irrational capi flow with

260a1e7

#17 fixed salt length setting when set to auto in cng-pss flow

f4fa051

oreoshake added the smimesign label Oct 1, 2021

lgarron closed this Oct 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PSS RSA support in darwin and windows #18

PSS RSA support in darwin and windows #18

avivklas commented Sep 1, 2020

piax93 commented Nov 5, 2020

btoews commented Nov 5, 2020

ptoomey3 commented Nov 5, 2020

vcsjones left a comment

vcsjones Nov 17, 2020

avivklas Jan 20, 2021

vcsjones Aug 26, 2021

vcsjones Nov 17, 2020

vcsjones Nov 17, 2020

avivklas Jan 20, 2021

avivklas left a comment

avivklas Jan 20, 2021

avivklas Jan 20, 2021

avivklas commented Feb 21, 2021

vcsjones left a comment

vcsjones Aug 26, 2021

lgarron commented Oct 4, 2021

PSS RSA support in darwin and windows #18

PSS RSA support in darwin and windows #18

Conversation

avivklas commented Sep 1, 2020

piax93 commented Nov 5, 2020

btoews commented Nov 5, 2020

ptoomey3 commented Nov 5, 2020

vcsjones left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avivklas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avivklas commented Feb 21, 2021

vcsjones left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgarron commented Oct 4, 2021