Add PKCS1v15 as a RSA signature and verification option on the Transit secret engine #4018

broamski · 2018-02-21T16:58:34Z

No description provided.

jefferai · 2018-02-21T17:27:45Z

What's the reason for this?

broamski · 2018-02-21T17:35:27Z

My use case is I have a number of existing processes using PKCS1v15 signatures that are not using Vault and I would really like for them to start using Vault!

joelthompson · 2018-02-21T17:46:30Z

Just throwing my $0.02 out here....

First, I think the option of signature type in the documentation should be clear that people should use PSS instead of PCKS1v15 unless they know what they're doing (or just omit it and leave it to be the default). Most people will be confused by the difference, and one thing Vault does well is it isolates people from needing to know the lower-level crypto details like the difference between PSS and PKCS1v15.

Second, it seems a bit kludgy to ask users to specify the signature type upon verify. Would it be possible to embed the signature type into the signed text returned by Vault so verify can just read it automatically?

broamski · 2018-02-21T17:55:26Z

PSS was and is still the default. This provides the option for those who need it. Both signature types are clearly supported in crypto/rsa, so this shouldn't be alarming.

jefferai · 2018-02-21T18:15:48Z

@broamski I'm not against adding it, although I think you're generally better off switching to PSS. I'm trying to think through the right place for this kind of option. It's definitely not that field, it's too single-purpose -- there's always the possibility that such a choice might become available for some other key type.

One possibility is simply a signature_algorithm field (and maybe renaming algorithm to hash_algorithm). Another might be to make it a configurable key property that you'd set with the keys/<key>/config endpoint. The benefit to doing that is that you take the onus off of the user to specify the parameter with every single sign/verify; the downside is you can't use the same key for both operations.

We could potentially do what Joel suggested and add it somehow to the output, although that would be a kind of a "spec change" for the output format.

broamski · 2018-02-21T18:33:03Z

In my experience, both signing and verification don't always happen on Vault; usually, privileged actors sign with vault and the other end verifies using any number of different libraries - sometimes later, while offline. I have received a few grumbles from implementers of the offline verification process about having to remove the vault:<version>: prefix. I feel avoiding any further "spec change" is the right approach.

As far as proper placement of the parameter, I vote for signature_algorithm and algorithm -> hash_algorithm.

joelthompson · 2018-02-21T21:10:48Z

PSS was and is still the default. This provides the option for those who need it. Both signature types are clearly supported in crypto/rsa, so this shouldn't be alarming.

Sure, but I can just see a lot of people asking the mailing list, "What is this signature_algorithm field? Which one should I use?" It'd be better to head off some of that by adding a little bit of information in the documentation, even if it's just something like, "If you don't have a need to explicitly set field, then leave it blank and accept the default."

jefferai · 2018-02-21T22:35:24Z

Any chance you want to implement the algorithm->hash_algorithm? :-)

It's easy -- mark the first as deprecated, but in the code, read the second, and if it's not set, read the first.

broamski · 2018-02-21T22:39:19Z

Of course! I'll try to get to it by later tonight/tomorrow morning.

broamski · 2018-02-22T18:28:09Z

Updated to reflect our latest consensus.

vishalnayak · 2018-02-24T10:45:20Z

builtin/logical/transit/path_sign_verify.go

-	algorithm := d.Get("urlalgorithm").(string)
-	if algorithm == "" {
-		algorithm = d.Get("algorithm").(string)
+	hashalgorithm := d.Get("urlalgorithm").(string)


Can we use casing for this var? hashalgorithm -> hashAlgorithm

vishalnayak · 2018-02-24T10:45:33Z

builtin/logical/transit/path_sign_verify.go

 	}
 	prehashed := d.Get("prehashed").(bool)
+	sigalgorithm := d.Get("signature_algorithm").(string)


s/sigalgorithm/sigAlgorithm

vishalnayak · 2018-02-24T10:47:01Z

helper/keysutil/policy.go

@@ -836,7 +836,7 @@ func (p *Policy) HMACKey(version int) ([]byte, error) {
 	return p.Keys[strconv.Itoa(version)].HMACKey, nil
 }

-func (p *Policy) Sign(ver int, context, input []byte, algorithm string) (*SigningResult, error) {
+func (p *Policy) Sign(ver int, context, input []byte, hashalgorithm string, sigalgorithm string) (*SigningResult, error) {


Specifying the type for hashAlgorithm can be skipped and hashAlgorithm, sigAlgorithm string should be sufficient.

vishalnayak · 2018-02-24T10:49:25Z

website/source/api/secret/transit/index.html.md

@@ -805,6 +805,12 @@ supports signing.
   hashed. If the key type is `rsa-2048` or `rsa-4096`, then the algorithm used
   to hash the input should be indicated by the `algorithm` parameter.

+- `rsa_sig_type` `(string: "pss")` – When using a RSA key, specifies the RSA


Needs to be updated.

broamski · 2018-02-26T15:02:01Z

Updated to address latest review.

jefferai · 2018-03-05T13:35:35Z

builtin/logical/transit/path_sign_verify.go

+			"algorithm": &framework.FieldSchema{
+				Type:    framework.TypeString,
+				Default: "sha2-256",
+				Description: `Deprecated: use "hash_algorithm" instead. Hash algorithm to use (POST body parameter). Valid values are:


I don't think you need to duplicate the help text here; should be sufficient to just mark it deprecated and to use hash_algorithm instead, since they can see the help text there, and the behavior for both parameters is exactly the same.

jefferai · 2018-03-05T13:36:37Z

builtin/logical/transit/path_sign_verify.go

@@ -63,6 +78,12 @@ to the min_encryption_version configured on the key.`,
 				Type:        framework.TypeBool,
 				Description: `Set to 'true' when the input is already hashed. If the key type is 'rsa-2048' or 'rsa-4096', then the algorithm used to hash the input should be indicated by the 'algorithm' parameter.`,
 			},
+			"signature_algorithm": &framework.FieldSchema{
+				Type:    framework.TypeString,
+				Default: "pss",


I don't think we should have a default, it ties this strongly to RSA. Instead I'd suggest the default behavior is just dependent on the type of key and this stays a blank string.

jefferai · 2018-03-05T13:37:19Z

builtin/logical/transit/path_sign_verify.go

-		algorithm = d.Get("algorithm").(string)
+	hashAlgorithm := d.Get("urlalgorithm").(string)
+	if hashAlgorithm == "" {
+		hashAlgorithm = d.Get("algorithm").(string)


Flip the order of these; prefer the non-deprecated one first.

… PKCS1v15

`hash_algorithm` in order support multiple rsa signature algorithms in the Tranit secret engine.

jefferai · 2018-03-15T16:16:48Z

Thanks!

jefferai added this to the 0.9.5 milestone Feb 22, 2018

vishalnayak reviewed Feb 24, 2018

View reviewed changes

jefferai modified the milestones: 0.9.5, 0.9.6 Feb 28, 2018

jefferai reviewed Mar 5, 2018

View reviewed changes

broamski added 7 commits March 7, 2018 16:53

Option to specify the RSA signature type, in specific add support for…

e27dfb0

… PKCS1v15

Update API documentation

45ae0cf

Add signature_algorithm field and update algorithm to

5ef21f7

`hash_algorithm` in order support multiple rsa signature algorithms in the Tranit secret engine.

Update variable case

f2c2073

Update documentation to reflect variable name changes

bdcdcc3

Eliminate duplicate description text, change order of parameter checking

aa5835a

Update data reference from algorithm to hash_algorithm in tests

ac41476

jefferai approved these changes Mar 15, 2018

View reviewed changes

jefferai merged commit ecb3fe2 into hashicorp:master Mar 15, 2018

jefferai added the ui label Apr 4, 2018

meirish mentioned this pull request Apr 6, 2018

UI - transit update #4298

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PKCS1v15 as a RSA signature and verification option on the Transit secret engine #4018

Add PKCS1v15 as a RSA signature and verification option on the Transit secret engine #4018

broamski commented Feb 21, 2018

jefferai commented Feb 21, 2018

broamski commented Feb 21, 2018

joelthompson commented Feb 21, 2018

broamski commented Feb 21, 2018

jefferai commented Feb 21, 2018

broamski commented Feb 21, 2018

joelthompson commented Feb 21, 2018

jefferai commented Feb 21, 2018

broamski commented Feb 21, 2018

broamski commented Feb 22, 2018

vishalnayak Feb 24, 2018

vishalnayak Feb 24, 2018

vishalnayak Feb 24, 2018

vishalnayak Feb 24, 2018

broamski commented Feb 26, 2018

jefferai Mar 5, 2018

jefferai Mar 5, 2018

jefferai Mar 5, 2018

jefferai commented Mar 15, 2018

Add PKCS1v15 as a RSA signature and verification option on the Transit secret engine #4018

Add PKCS1v15 as a RSA signature and verification option on the Transit secret engine #4018

Conversation

broamski commented Feb 21, 2018

jefferai commented Feb 21, 2018

broamski commented Feb 21, 2018

joelthompson commented Feb 21, 2018

broamski commented Feb 21, 2018

jefferai commented Feb 21, 2018

broamski commented Feb 21, 2018

joelthompson commented Feb 21, 2018

jefferai commented Feb 21, 2018

broamski commented Feb 21, 2018

broamski commented Feb 22, 2018

vishalnayak Feb 24, 2018

Choose a reason for hiding this comment

vishalnayak Feb 24, 2018

Choose a reason for hiding this comment

vishalnayak Feb 24, 2018

Choose a reason for hiding this comment

vishalnayak Feb 24, 2018

Choose a reason for hiding this comment

broamski commented Feb 26, 2018

jefferai Mar 5, 2018

Choose a reason for hiding this comment

jefferai Mar 5, 2018

Choose a reason for hiding this comment

jefferai Mar 5, 2018

Choose a reason for hiding this comment

jefferai commented Mar 15, 2018