Integrate OAEP and PSS into existing interface

[dignifiedquire]

This integrates #18 and #26 by expanding the use of PaddingScheme to capture all optional values for the different schemes.

It is not ideal, but it works well and keeps in line with the existing API for now. I think we should still continue to improve on the api and think through #34 more, but in the meantime this a version that folks can start to use, and isn't too different from the existing api

[str4d]

Hmm, pulling in 2462f1d from #18 goes in the opposite direction to #42. It looks like the main thing it does (remove PublicKey trait) is not present in the overall PR, and I can't find where that is undone, but I think it's in the refactor commit? GitHub isn't helping much here. There are other effects of that commit that are not undone (in particular, it makes public keys non-generic in pkcs1v15, the opposite of what I've done in #44).

How painful would it be to just drop that commit entirely? It would likely make the history clearer.

[str4d]

Hmm, the change in that commit to store an RSAPublicKey inside RSAPrivateKey does make sense to me. I'd personally alter that commit to only contain the pieces we want, but that's probably more hassle than it's worth, so just reverting changes we don't want is probably fine.

[dignifiedquire]

Yeah, I changed a lot of things during the merge, which is..unfortunate. So at this point I am thinking of squashing this into two commits at the end likely (1) for oaep and (2) for pss, after doing/undoing all things we want to do.

[dignifiedquire]

@str4d I made the OAEP and PSS impls generic over the Key traits now

[roblabla]

Hey that looks great! I have a couple suggestions

[roblabla]

Could make it Box<dyn DynDigest + Debug> if really necessary. AFAIK all the digest crates have debug unconditionally implemented on them.

[dignifiedquire]

that doesn't work, the compiler is upset about more than one non auto trait :(

[roblabla]

Ah right, it's possible to make your own trait (e.g. trait MyDynDigest: DynDigest + Debug {}) and then use that as a Box<dyn MyDynDigest>, but at this point it's too much boilerplate just for the sake of getting the digest name to show IMO.

[tarcieri]

Relevant to the PSS use case: final two week comment period for shipping signature 1.0: RustCrypto/traits#78

[str4d]

Now that we have PrivateKey: PublicKeyParts, this impl Deref should be removed.

[dignifiedquire]

Not quite, removing this, removes the ability to call .verify on a private key

[str4d]

Should this check also be in raw_decryption_primitive?

[dignifiedquire]

@str4d thanks for the review, I think I addressed all important things

[str4d]

The hash: None case appears to still be necessary for unpadded signatures (at least, that's what the test case in src/pkcs1v15.rs leads me to believe).

[dignifiedquire]

Yes, which is why it is still an Option, not sure what you mean.

[str4d]

I was just noting this because it wasn't immediately obvious during review that the Option<Hash> was necessary besides enabling the prior encryption case to specify None.

[str4d]

I made a close review pass over the RSAES-OAEP implementation, comparing it against RFC 8017 section 7.1.

[dignifiedquire]

@str4d applied the new round of CR

[str4d]

These are causing the current CI failure.

[str4d]

I made a close review pass over the RSASSA-PSS implementation. The relevant sections of RFC 8017 were already inlined as comments.

[str4d]

Is there precedent for detecting the salt length? I know it is possible due to the structure of DB, but there is no mention of doing so in RFC 8017. I can't read the IEEE documents that reference different possible salt lengths, which maybe discusses this. As is, we never exercise the explicit-salt-length case below.

[dignifiedquire]

I don't know @roblabla I believe you wrote this code initially, do you have any answer to this?

[str4d]

Is there precedent for this default salt length? The RFC mentions that common choices are digest.output_size() and 0, while this is selecting the salt to be as large as could fit. I can't read the IEEE documents that reference different possible salt lengths. In any case, I don't think there's a problem with this, given that we are detecting the salt length in verify.

[str4d]

Length is not checked, but with the current API we never actually see the message.

[str4d]

Length is not checked, but with the current API we never actually see the message.

[str4d]

This would be clearer (and closer to the spec):

Suggested change

	let h = &mut h[..(em_len - 1) - (em_len - h_len - 1)];
	let h = &mut h[..h_len];

[dignifiedquire]

fixed

[str4d]

Suggested change

	None => (0..=em_len - (h_len + 2))
	.rev()
	.try_fold(None, |state, i| match(state, db[em_len - h_len - i - 2]){
	None => (0..em_len - h_len - 1)
	.try_fold(None, |state, i| match(state, db[i]){

[dignifiedquire]

this change results in failures, haven't debugged why, gonna leave the logic as is for now

[str4d]

Suggested change

	for e in&db[..em_len - h_len - s_len - 2]{
	if*e != 0x00{
	returnErr(Error::Verification);
	}
	}
	if db[em_len - h_len - s_len - 2] != 0x01{
	returnErr(Error::Verification);
	}
	let(zeroes, rest) = db.split_at(em_len - h_len - s_len - 2);
	if zeroes.iter().any(|e| *e != 0x00) || rest[0] != 0x01{
	returnErr(Error::Verification);
	}

[dignifiedquire]

fixed

[str4d]

Suggested change

	ifInto::<bool>::into(h0.ct_eq(h)){
	if h0.ct_eq(h).into(){

[dignifiedquire]

fixed

[str4d]

Suggested change

	let(db, h) = em.split_at_mut(em_len - s_len - h_len - 2 + 1 + s_len);
	let(db, h) = em.split_at_mut(em_len - h_len - 1);

[dignifiedquire]

fixed

[str4d]

We're assuming here that the em buffer passed into emsa_pss_encode is already zeroed. This happens to be the case, but given that emsa_pss_encode is only called from one place, and this PR isn't attempting to be no-std compatible, it would be clearer if em was constructed and returned from this function. Non-blocking.

[dignifiedquire]

fixed

[dignifiedquire]

This integrates #18 and #26 by expanding the use of PaddingScheme to capture all optional values for the different schemes.

It is not ideal, but it works well and keeps in line with the existing API for now. I think we should still continue to improve on the api and think through #34 more, but in the meantime this a version that folks can start to use, and isn't too different from the existing api

[str4d]

Hmm, pulling in 2462f1d from #18 goes in the opposite direction to #42. It looks like the main thing it does (remove PublicKey trait) is not present in the overall PR, and I can't find where that is undone, but I think it's in the refactor commit? GitHub isn't helping much here. There are other effects of that commit that are not undone (in particular, it makes public keys non-generic in pkcs1v15, the opposite of what I've done in #44).

How painful would it be to just drop that commit entirely? It would likely make the history clearer.

[str4d]

Hmm, the change in that commit to store an RSAPublicKey inside RSAPrivateKey does make sense to me. I'd personally alter that commit to only contain the pieces we want, but that's probably more hassle than it's worth, so just reverting changes we don't want is probably fine.

[dignifiedquire]

Yeah, I changed a lot of things during the merge, which is..unfortunate. So at this point I am thinking of squashing this into two commits at the end likely (1) for oaep and (2) for pss, after doing/undoing all things we want to do.

[dignifiedquire]

@str4d I made the OAEP and PSS impls generic over the Key traits now

[roblabla]

Hey that looks great! I have a couple suggestions

[roblabla]

Could make it Box<dyn DynDigest + Debug> if really necessary. AFAIK all the digest crates have debug unconditionally implemented on them.

[dignifiedquire]

that doesn't work, the compiler is upset about more than one non auto trait :(

[roblabla]

Ah right, it's possible to make your own trait (e.g. trait MyDynDigest: DynDigest + Debug {}) and then use that as a Box<dyn MyDynDigest>, but at this point it's too much boilerplate just for the sake of getting the digest name to show IMO.

[tarcieri]

Relevant to the PSS use case: final two week comment period for shipping signature 1.0: RustCrypto/traits#78

[str4d]

Now that we have PrivateKey: PublicKeyParts, this impl Deref should be removed.

[dignifiedquire]

Not quite, removing this, removes the ability to call .verify on a private key

[str4d]

Should this check also be in raw_decryption_primitive?

[dignifiedquire]

@str4d thanks for the review, I think I addressed all important things

[str4d]

The hash: None case appears to still be necessary for unpadded signatures (at least, that's what the test case in src/pkcs1v15.rs leads me to believe).

[dignifiedquire]

Yes, which is why it is still an Option, not sure what you mean.

[str4d]

I was just noting this because it wasn't immediately obvious during review that the Option<Hash> was necessary besides enabling the prior encryption case to specify None.

[str4d]

I made a close review pass over the RSAES-OAEP implementation, comparing it against RFC 8017 section 7.1.

[dignifiedquire]

@str4d applied the new round of CR

[str4d]

These are causing the current CI failure.

[str4d]

I made a close review pass over the RSASSA-PSS implementation. The relevant sections of RFC 8017 were already inlined as comments.

[str4d]

Is there precedent for detecting the salt length? I know it is possible due to the structure of DB, but there is no mention of doing so in RFC 8017. I can't read the IEEE documents that reference different possible salt lengths, which maybe discusses this. As is, we never exercise the explicit-salt-length case below.

[dignifiedquire]

I don't know @roblabla I believe you wrote this code initially, do you have any answer to this?

[str4d]

Is there precedent for this default salt length? The RFC mentions that common choices are digest.output_size() and 0, while this is selecting the salt to be as large as could fit. I can't read the IEEE documents that reference different possible salt lengths. In any case, I don't think there's a problem with this, given that we are detecting the salt length in verify.

[str4d]

Length is not checked, but with the current API we never actually see the message.

[str4d]

Length is not checked, but with the current API we never actually see the message.

[str4d]

This would be clearer (and closer to the spec):

Suggested change

	let h = &mut h[..(em_len - 1) - (em_len - h_len - 1)];
	let h = &mut h[..h_len];

[dignifiedquire]

fixed

[str4d]

Suggested change

	None => (0..=em_len - (h_len + 2))
	.rev()
	.try_fold(None, |state, i| match(state, db[em_len - h_len - i - 2]){
	None => (0..em_len - h_len - 1)
	.try_fold(None, |state, i| match(state, db[i]){

[dignifiedquire]

this change results in failures, haven't debugged why, gonna leave the logic as is for now

[str4d]

Suggested change

	for e in&db[..em_len - h_len - s_len - 2]{
	if*e != 0x00{
	returnErr(Error::Verification);
	}
	}
	if db[em_len - h_len - s_len - 2] != 0x01{
	returnErr(Error::Verification);
	}
	let(zeroes, rest) = db.split_at(em_len - h_len - s_len - 2);
	if zeroes.iter().any(|e| *e != 0x00) || rest[0] != 0x01{
	returnErr(Error::Verification);
	}

[dignifiedquire]

fixed

[str4d]

Suggested change

	ifInto::<bool>::into(h0.ct_eq(h)){
	if h0.ct_eq(h).into(){

[dignifiedquire]

fixed

[str4d]

Suggested change

	let(db, h) = em.split_at_mut(em_len - s_len - h_len - 2 + 1 + s_len);
	let(db, h) = em.split_at_mut(em_len - h_len - 1);

[dignifiedquire]

fixed

[str4d]

We're assuming here that the em buffer passed into emsa_pss_encode is already zeroed. This happens to be the case, but given that emsa_pss_encode is only called from one place, and this PR isn't attempting to be no-std compatible, it would be clearer if em was constructed and returned from this function. Non-blocking.

[dignifiedquire]

fixed

[dignifiedquire]

This integrates #18 and #26 by expanding the use of PaddingScheme to capture all optional values for the different schemes.

It is not ideal, but it works well and keeps in line with the existing API for now. I think we should still continue to improve on the api and think through #34 more, but in the meantime this a version that folks can start to use, and isn't too different from the existing api

[str4d]

Hmm, pulling in 2462f1d from #18 goes in the opposite direction to #42. It looks like the main thing it does (remove PublicKey trait) is not present in the overall PR, and I can't find where that is undone, but I think it's in the refactor commit? GitHub isn't helping much here. There are other effects of that commit that are not undone (in particular, it makes public keys non-generic in pkcs1v15, the opposite of what I've done in #44).

How painful would it be to just drop that commit entirely? It would likely make the history clearer.

[str4d]

Hmm, the change in that commit to store an RSAPublicKey inside RSAPrivateKey does make sense to me. I'd personally alter that commit to only contain the pieces we want, but that's probably more hassle than it's worth, so just reverting changes we don't want is probably fine.

[dignifiedquire]

Yeah, I changed a lot of things during the merge, which is..unfortunate. So at this point I am thinking of squashing this into two commits at the end likely (1) for oaep and (2) for pss, after doing/undoing all things we want to do.

[dignifiedquire]

@str4d I made the OAEP and PSS impls generic over the Key traits now

[roblabla]

Hey that looks great! I have a couple suggestions

[roblabla]

Could make it Box<dyn DynDigest + Debug> if really necessary. AFAIK all the digest crates have debug unconditionally implemented on them.

[dignifiedquire]

that doesn't work, the compiler is upset about more than one non auto trait :(

[roblabla]

Ah right, it's possible to make your own trait (e.g. trait MyDynDigest: DynDigest + Debug {}) and then use that as a Box<dyn MyDynDigest>, but at this point it's too much boilerplate just for the sake of getting the digest name to show IMO.

[tarcieri]

Relevant to the PSS use case: final two week comment period for shipping signature 1.0: RustCrypto/traits#78

[str4d]

Now that we have PrivateKey: PublicKeyParts, this impl Deref should be removed.

[dignifiedquire]

Not quite, removing this, removes the ability to call .verify on a private key

[str4d]

Should this check also be in raw_decryption_primitive?

[dignifiedquire]

@str4d thanks for the review, I think I addressed all important things

[str4d]

The hash: None case appears to still be necessary for unpadded signatures (at least, that's what the test case in src/pkcs1v15.rs leads me to believe).

[dignifiedquire]

Yes, which is why it is still an Option, not sure what you mean.

[str4d]

I was just noting this because it wasn't immediately obvious during review that the Option<Hash> was necessary besides enabling the prior encryption case to specify None.

[str4d]

I made a close review pass over the RSAES-OAEP implementation, comparing it against RFC 8017 section 7.1.

[dignifiedquire]

@str4d applied the new round of CR

[str4d]

These are causing the current CI failure.

[str4d]

I made a close review pass over the RSASSA-PSS implementation. The relevant sections of RFC 8017 were already inlined as comments.

[str4d]

Is there precedent for detecting the salt length? I know it is possible due to the structure of DB, but there is no mention of doing so in RFC 8017. I can't read the IEEE documents that reference different possible salt lengths, which maybe discusses this. As is, we never exercise the explicit-salt-length case below.

[dignifiedquire]

I don't know @roblabla I believe you wrote this code initially, do you have any answer to this?

[str4d]

Is there precedent for this default salt length? The RFC mentions that common choices are digest.output_size() and 0, while this is selecting the salt to be as large as could fit. I can't read the IEEE documents that reference different possible salt lengths. In any case, I don't think there's a problem with this, given that we are detecting the salt length in verify.

[str4d]

Length is not checked, but with the current API we never actually see the message.

[str4d]

Length is not checked, but with the current API we never actually see the message.

[str4d]

This would be clearer (and closer to the spec):

Suggested change

	let h = &mut h[..(em_len - 1) - (em_len - h_len - 1)];
	let h = &mut h[..h_len];

[dignifiedquire]

fixed

[str4d]

Suggested change

	None => (0..=em_len - (h_len + 2))
	.rev()
	.try_fold(None, |state, i| match(state, db[em_len - h_len - i - 2]){
	None => (0..em_len - h_len - 1)
	.try_fold(None, |state, i| match(state, db[i]){

[dignifiedquire]

this change results in failures, haven't debugged why, gonna leave the logic as is for now

[str4d]

Suggested change

	for e in&db[..em_len - h_len - s_len - 2]{
	if*e != 0x00{
	returnErr(Error::Verification);
	}
	}
	if db[em_len - h_len - s_len - 2] != 0x01{
	returnErr(Error::Verification);
	}
	let(zeroes, rest) = db.split_at(em_len - h_len - s_len - 2);
	if zeroes.iter().any(|e| *e != 0x00) || rest[0] != 0x01{
	returnErr(Error::Verification);
	}

[dignifiedquire]

fixed

[str4d]

Suggested change

	ifInto::<bool>::into(h0.ct_eq(h)){
	if h0.ct_eq(h).into(){

[dignifiedquire]

fixed

[str4d]

Suggested change

	let(db, h) = em.split_at_mut(em_len - s_len - h_len - 2 + 1 + s_len);
	let(db, h) = em.split_at_mut(em_len - h_len - 1);

[dignifiedquire]

fixed

[str4d]

We're assuming here that the em buffer passed into emsa_pss_encode is already zeroed. This happens to be the case, but given that emsa_pss_encode is only called from one place, and this PR isn't attempting to be no-std compatible, it would be clearer if em was constructed and returned from this function. Non-blocking.

[dignifiedquire]

fixed

[dignifiedquire]

This integrates #18 and #26 by expanding the use of PaddingScheme to capture all optional values for the different schemes.

It is not ideal, but it works well and keeps in line with the existing API for now. I think we should still continue to improve on the api and think through #34 more, but in the meantime this a version that folks can start to use, and isn't too different from the existing api

[str4d]

Hmm, pulling in 2462f1d from #18 goes in the opposite direction to #42. It looks like the main thing it does (remove PublicKey trait) is not present in the overall PR, and I can't find where that is undone, but I think it's in the refactor commit? GitHub isn't helping much here. There are other effects of that commit that are not undone (in particular, it makes public keys non-generic in pkcs1v15, the opposite of what I've done in #44).

How painful would it be to just drop that commit entirely? It would likely make the history clearer.

[str4d]

Hmm, the change in that commit to store an RSAPublicKey inside RSAPrivateKey does make sense to me. I'd personally alter that commit to only contain the pieces we want, but that's probably more hassle than it's worth, so just reverting changes we don't want is probably fine.

[dignifiedquire]

Yeah, I changed a lot of things during the merge, which is..unfortunate. So at this point I am thinking of squashing this into two commits at the end likely (1) for oaep and (2) for pss, after doing/undoing all things we want to do.

[dignifiedquire]

@str4d I made the OAEP and PSS impls generic over the Key traits now

[roblabla]

Hey that looks great! I have a couple suggestions

[roblabla]

Could make it Box<dyn DynDigest + Debug> if really necessary. AFAIK all the digest crates have debug unconditionally implemented on them.

[dignifiedquire]

that doesn't work, the compiler is upset about more than one non auto trait :(

[roblabla]

Ah right, it's possible to make your own trait (e.g. trait MyDynDigest: DynDigest + Debug {}) and then use that as a Box<dyn MyDynDigest>, but at this point it's too much boilerplate just for the sake of getting the digest name to show IMO.

[tarcieri]

Relevant to the PSS use case: final two week comment period for shipping signature 1.0: RustCrypto/traits#78

[str4d]

Now that we have PrivateKey: PublicKeyParts, this impl Deref should be removed.

[dignifiedquire]

Not quite, removing this, removes the ability to call .verify on a private key

[str4d]

Should this check also be in raw_decryption_primitive?

[dignifiedquire]

@str4d thanks for the review, I think I addressed all important things

[str4d]

The hash: None case appears to still be necessary for unpadded signatures (at least, that's what the test case in src/pkcs1v15.rs leads me to believe).

[dignifiedquire]

Yes, which is why it is still an Option, not sure what you mean.

[str4d]

I was just noting this because it wasn't immediately obvious during review that the Option<Hash> was necessary besides enabling the prior encryption case to specify None.

[str4d]

I made a close review pass over the RSAES-OAEP implementation, comparing it against RFC 8017 section 7.1.

[dignifiedquire]

@str4d applied the new round of CR

[str4d]

These are causing the current CI failure.

[str4d]

I made a close review pass over the RSASSA-PSS implementation. The relevant sections of RFC 8017 were already inlined as comments.

[str4d]

Is there precedent for detecting the salt length? I know it is possible due to the structure of DB, but there is no mention of doing so in RFC 8017. I can't read the IEEE documents that reference different possible salt lengths, which maybe discusses this. As is, we never exercise the explicit-salt-length case below.

[dignifiedquire]

I don't know @roblabla I believe you wrote this code initially, do you have any answer to this?

[str4d]

Is there precedent for this default salt length? The RFC mentions that common choices are digest.output_size() and 0, while this is selecting the salt to be as large as could fit. I can't read the IEEE documents that reference different possible salt lengths. In any case, I don't think there's a problem with this, given that we are detecting the salt length in verify.

[str4d]

Length is not checked, but with the current API we never actually see the message.

[str4d]

Length is not checked, but with the current API we never actually see the message.

[str4d]

This would be clearer (and closer to the spec):

Suggested change

	let h = &mut h[..(em_len - 1) - (em_len - h_len - 1)];
	let h = &mut h[..h_len];

[dignifiedquire]

fixed

[str4d]

Suggested change

	None => (0..=em_len - (h_len + 2))
	.rev()
	.try_fold(None, |state, i| match(state, db[em_len - h_len - i - 2]){
	None => (0..em_len - h_len - 1)
	.try_fold(None, |state, i| match(state, db[i]){

[dignifiedquire]

this change results in failures, haven't debugged why, gonna leave the logic as is for now

[str4d]

Suggested change

	for e in&db[..em_len - h_len - s_len - 2]{
	if*e != 0x00{
	returnErr(Error::Verification);
	}
	}
	if db[em_len - h_len - s_len - 2] != 0x01{
	returnErr(Error::Verification);
	}
	let(zeroes, rest) = db.split_at(em_len - h_len - s_len - 2);
	if zeroes.iter().any(|e| *e != 0x00) || rest[0] != 0x01{
	returnErr(Error::Verification);
	}

[dignifiedquire]

fixed

[str4d]

Suggested change

	ifInto::<bool>::into(h0.ct_eq(h)){
	if h0.ct_eq(h).into(){

[dignifiedquire]

fixed

[str4d]

Suggested change

	let(db, h) = em.split_at_mut(em_len - s_len - h_len - 2 + 1 + s_len);
	let(db, h) = em.split_at_mut(em_len - h_len - 1);

[dignifiedquire]

fixed

[str4d]

We're assuming here that the em buffer passed into emsa_pss_encode is already zeroed. This happens to be the case, but given that emsa_pss_encode is only called from one place, and this PR isn't attempting to be no-std compatible, it would be clearer if em was constructed and returned from this function. Non-blocking.

[dignifiedquire]

fixed

[dignifiedquire]

This integrates #18 and #26 by expanding the use of PaddingScheme to capture all optional values for the different schemes.

It is not ideal, but it works well and keeps in line with the existing API for now. I think we should still continue to improve on the api and think through #34 more, but in the meantime this a version that folks can start to use, and isn't too different from the existing api

[str4d]

Hmm, pulling in 2462f1d from #18 goes in the opposite direction to #42. It looks like the main thing it does (remove PublicKey trait) is not present in the overall PR, and I can't find where that is undone, but I think it's in the refactor commit? GitHub isn't helping much here. There are other effects of that commit that are not undone (in particular, it makes public keys non-generic in pkcs1v15, the opposite of what I've done in #44).

How painful would it be to just drop that commit entirely? It would likely make the history clearer.

[str4d]

Hmm, the change in that commit to store an RSAPublicKey inside RSAPrivateKey does make sense to me. I'd personally alter that commit to only contain the pieces we want, but that's probably more hassle than it's worth, so just reverting changes we don't want is probably fine.

[dignifiedquire]

Yeah, I changed a lot of things during the merge, which is..unfortunate. So at this point I am thinking of squashing this into two commits at the end likely (1) for oaep and (2) for pss, after doing/undoing all things we want to do.

[dignifiedquire]

@str4d I made the OAEP and PSS impls generic over the Key traits now

[roblabla]

Hey that looks great! I have a couple suggestions

[roblabla]

Could make it Box<dyn DynDigest + Debug> if really necessary. AFAIK all the digest crates have debug unconditionally implemented on them.

[dignifiedquire]

that doesn't work, the compiler is upset about more than one non auto trait :(

[roblabla]

Ah right, it's possible to make your own trait (e.g. trait MyDynDigest: DynDigest + Debug {}) and then use that as a Box<dyn MyDynDigest>, but at this point it's too much boilerplate just for the sake of getting the digest name to show IMO.

[tarcieri]

Relevant to the PSS use case: final two week comment period for shipping signature 1.0: RustCrypto/traits#78

[str4d]

Now that we have PrivateKey: PublicKeyParts, this impl Deref should be removed.

[dignifiedquire]

Not quite, removing this, removes the ability to call .verify on a private key

[str4d]

Should this check also be in raw_decryption_primitive?

[dignifiedquire]

@str4d thanks for the review, I think I addressed all important things

[str4d]

The hash: None case appears to still be necessary for unpadded signatures (at least, that's what the test case in src/pkcs1v15.rs leads me to believe).

[dignifiedquire]

Yes, which is why it is still an Option, not sure what you mean.

[str4d]

I was just noting this because it wasn't immediately obvious during review that the Option<Hash> was necessary besides enabling the prior encryption case to specify None.

[str4d]

I made a close review pass over the RSAES-OAEP implementation, comparing it against RFC 8017 section 7.1.

[dignifiedquire]

@str4d applied the new round of CR

[str4d]

These are causing the current CI failure.

[str4d]

I made a close review pass over the RSASSA-PSS implementation. The relevant sections of RFC 8017 were already inlined as comments.

[str4d]

Is there precedent for detecting the salt length? I know it is possible due to the structure of DB, but there is no mention of doing so in RFC 8017. I can't read the IEEE documents that reference different possible salt lengths, which maybe discusses this. As is, we never exercise the explicit-salt-length case below.

[dignifiedquire]

I don't know @roblabla I believe you wrote this code initially, do you have any answer to this?

[str4d]

Is there precedent for this default salt length? The RFC mentions that common choices are digest.output_size() and 0, while this is selecting the salt to be as large as could fit. I can't read the IEEE documents that reference different possible salt lengths. In any case, I don't think there's a problem with this, given that we are detecting the salt length in verify.

[str4d]

Length is not checked, but with the current API we never actually see the message.

[str4d]

Length is not checked, but with the current API we never actually see the message.

[str4d]

This would be clearer (and closer to the spec):

Suggested change

	let h = &mut h[..(em_len - 1) - (em_len - h_len - 1)];
	let h = &mut h[..h_len];

[dignifiedquire]

fixed

[str4d]

Suggested change

	None => (0..=em_len - (h_len + 2))
	.rev()
	.try_fold(None, |state, i| match(state, db[em_len - h_len - i - 2]){
	None => (0..em_len - h_len - 1)
	.try_fold(None, |state, i| match(state, db[i]){

[dignifiedquire]

this change results in failures, haven't debugged why, gonna leave the logic as is for now

[str4d]

Suggested change

	for e in&db[..em_len - h_len - s_len - 2]{
	if*e != 0x00{
	returnErr(Error::Verification);
	}
	}
	if db[em_len - h_len - s_len - 2] != 0x01{
	returnErr(Error::Verification);
	}
	let(zeroes, rest) = db.split_at(em_len - h_len - s_len - 2);
	if zeroes.iter().any(|e| *e != 0x00) || rest[0] != 0x01{
	returnErr(Error::Verification);
	}

[dignifiedquire]

fixed

[str4d]

Suggested change

	ifInto::<bool>::into(h0.ct_eq(h)){
	if h0.ct_eq(h).into(){

[dignifiedquire]

fixed

[str4d]

Suggested change

	let(db, h) = em.split_at_mut(em_len - s_len - h_len - 2 + 1 + s_len);
	let(db, h) = em.split_at_mut(em_len - h_len - 1);

[dignifiedquire]

fixed

[str4d]

We're assuming here that the em buffer passed into emsa_pss_encode is already zeroed. This happens to be the case, but given that emsa_pss_encode is only called from one place, and this PR isn't attempting to be no-std compatible, it would be clearer if em was constructed and returned from this function. Non-blocking.

[dignifiedquire]

fixed

[dignifiedquire]

This integrates #18 and #26 by expanding the use of PaddingScheme to capture all optional values for the different schemes.

It is not ideal, but it works well and keeps in line with the existing API for now. I think we should still continue to improve on the api and think through #34 more, but in the meantime this a version that folks can start to use, and isn't too different from the existing api

[str4d]

Hmm, pulling in 2462f1d from #18 goes in the opposite direction to #42. It looks like the main thing it does (remove PublicKey trait) is not present in the overall PR, and I can't find where that is undone, but I think it's in the refactor commit? GitHub isn't helping much here. There are other effects of that commit that are not undone (in particular, it makes public keys non-generic in pkcs1v15, the opposite of what I've done in #44).

How painful would it be to just drop that commit entirely? It would likely make the history clearer.

[str4d]

Hmm, the change in that commit to store an RSAPublicKey inside RSAPrivateKey does make sense to me. I'd personally alter that commit to only contain the pieces we want, but that's probably more hassle than it's worth, so just reverting changes we don't want is probably fine.

[dignifiedquire]

Yeah, I changed a lot of things during the merge, which is..unfortunate. So at this point I am thinking of squashing this into two commits at the end likely (1) for oaep and (2) for pss, after doing/undoing all things we want to do.

[dignifiedquire]

@str4d I made the OAEP and PSS impls generic over the Key traits now

[roblabla]

Hey that looks great! I have a couple suggestions

[roblabla]

Could make it Box<dyn DynDigest + Debug> if really necessary. AFAIK all the digest crates have debug unconditionally implemented on them.

[dignifiedquire]

that doesn't work, the compiler is upset about more than one non auto trait :(

[roblabla]

Ah right, it's possible to make your own trait (e.g. trait MyDynDigest: DynDigest + Debug {}) and then use that as a Box<dyn MyDynDigest>, but at this point it's too much boilerplate just for the sake of getting the digest name to show IMO.

[tarcieri]

Relevant to the PSS use case: final two week comment period for shipping signature 1.0: RustCrypto/traits#78

[str4d]

Now that we have PrivateKey: PublicKeyParts, this impl Deref should be removed.

[dignifiedquire]

Not quite, removing this, removes the ability to call .verify on a private key

[str4d]

Should this check also be in raw_decryption_primitive?

[dignifiedquire]

@str4d thanks for the review, I think I addressed all important things

[str4d]

The hash: None case appears to still be necessary for unpadded signatures (at least, that's what the test case in src/pkcs1v15.rs leads me to believe).

[dignifiedquire]

Yes, which is why it is still an Option, not sure what you mean.

[str4d]

I was just noting this because it wasn't immediately obvious during review that the Option<Hash> was necessary besides enabling the prior encryption case to specify None.

[str4d]

I made a close review pass over the RSAES-OAEP implementation, comparing it against RFC 8017 section 7.1.

[dignifiedquire]

@str4d applied the new round of CR

[str4d]

These are causing the current CI failure.

[str4d]

I made a close review pass over the RSASSA-PSS implementation. The relevant sections of RFC 8017 were already inlined as comments.

[str4d]

Is there precedent for detecting the salt length? I know it is possible due to the structure of DB, but there is no mention of doing so in RFC 8017. I can't read the IEEE documents that reference different possible salt lengths, which maybe discusses this. As is, we never exercise the explicit-salt-length case below.

[dignifiedquire]

I don't know @roblabla I believe you wrote this code initially, do you have any answer to this?

[str4d]

Is there precedent for this default salt length? The RFC mentions that common choices are digest.output_size() and 0, while this is selecting the salt to be as large as could fit. I can't read the IEEE documents that reference different possible salt lengths. In any case, I don't think there's a problem with this, given that we are detecting the salt length in verify.

[str4d]

Length is not checked, but with the current API we never actually see the message.

[str4d]

Length is not checked, but with the current API we never actually see the message.

[str4d]

This would be clearer (and closer to the spec):

Suggested change

	let h = &mut h[..(em_len - 1) - (em_len - h_len - 1)];
	let h = &mut h[..h_len];

[dignifiedquire]

fixed

[str4d]

Suggested change

	None => (0..=em_len - (h_len + 2))
	.rev()
	.try_fold(None, |state, i| match(state, db[em_len - h_len - i - 2]){
	None => (0..em_len - h_len - 1)
	.try_fold(None, |state, i| match(state, db[i]){

[dignifiedquire]

this change results in failures, haven't debugged why, gonna leave the logic as is for now

[str4d]

Suggested change

	for e in&db[..em_len - h_len - s_len - 2]{
	if*e != 0x00{
	returnErr(Error::Verification);
	}
	}
	if db[em_len - h_len - s_len - 2] != 0x01{
	returnErr(Error::Verification);
	}
	let(zeroes, rest) = db.split_at(em_len - h_len - s_len - 2);
	if zeroes.iter().any(|e| *e != 0x00) || rest[0] != 0x01{
	returnErr(Error::Verification);
	}

[dignifiedquire]

fixed

[str4d]

Suggested change

	ifInto::<bool>::into(h0.ct_eq(h)){
	if h0.ct_eq(h).into(){

[dignifiedquire]

fixed

[str4d]

Suggested change

	let(db, h) = em.split_at_mut(em_len - s_len - h_len - 2 + 1 + s_len);
	let(db, h) = em.split_at_mut(em_len - h_len - 1);

[dignifiedquire]

fixed

[str4d]

We're assuming here that the em buffer passed into emsa_pss_encode is already zeroed. This happens to be the case, but given that emsa_pss_encode is only called from one place, and this PR isn't attempting to be no-std compatible, it would be clearer if em was constructed and returned from this function. Non-blocking.

[dignifiedquire]

fixed

[dignifiedquire]

This integrates #18 and #26 by expanding the use of PaddingScheme to capture all optional values for the different schemes.

It is not ideal, but it works well and keeps in line with the existing API for now. I think we should still continue to improve on the api and think through #34 more, but in the meantime this a version that folks can start to use, and isn't too different from the existing api

[str4d]

Hmm, pulling in 2462f1d from #18 goes in the opposite direction to #42. It looks like the main thing it does (remove PublicKey trait) is not present in the overall PR, and I can't find where that is undone, but I think it's in the refactor commit? GitHub isn't helping much here. There are other effects of that commit that are not undone (in particular, it makes public keys non-generic in pkcs1v15, the opposite of what I've done in #44).

How painful would it be to just drop that commit entirely? It would likely make the history clearer.

[str4d]

Hmm, the change in that commit to store an RSAPublicKey inside RSAPrivateKey does make sense to me. I'd personally alter that commit to only contain the pieces we want, but that's probably more hassle than it's worth, so just reverting changes we don't want is probably fine.

[dignifiedquire]

Yeah, I changed a lot of things during the merge, which is..unfortunate. So at this point I am thinking of squashing this into two commits at the end likely (1) for oaep and (2) for pss, after doing/undoing all things we want to do.

[dignifiedquire]

@str4d I made the OAEP and PSS impls generic over the Key traits now

[roblabla]

Hey that looks great! I have a couple suggestions

[roblabla]

Could make it Box<dyn DynDigest + Debug> if really necessary. AFAIK all the digest crates have debug unconditionally implemented on them.

[dignifiedquire]

that doesn't work, the compiler is upset about more than one non auto trait :(

[roblabla]

Ah right, it's possible to make your own trait (e.g. trait MyDynDigest: DynDigest + Debug {}) and then use that as a Box<dyn MyDynDigest>, but at this point it's too much boilerplate just for the sake of getting the digest name to show IMO.

[tarcieri]

Relevant to the PSS use case: final two week comment period for shipping signature 1.0: RustCrypto/traits#78

[str4d]

Now that we have PrivateKey: PublicKeyParts, this impl Deref should be removed.

[dignifiedquire]

Not quite, removing this, removes the ability to call .verify on a private key

[str4d]

Should this check also be in raw_decryption_primitive?

[dignifiedquire]

@str4d thanks for the review, I think I addressed all important things

[str4d]

The hash: None case appears to still be necessary for unpadded signatures (at least, that's what the test case in src/pkcs1v15.rs leads me to believe).

[dignifiedquire]

Yes, which is why it is still an Option, not sure what you mean.

[str4d]

I was just noting this because it wasn't immediately obvious during review that the Option<Hash> was necessary besides enabling the prior encryption case to specify None.

[str4d]

I made a close review pass over the RSAES-OAEP implementation, comparing it against RFC 8017 section 7.1.

[dignifiedquire]

@str4d applied the new round of CR

[str4d]

These are causing the current CI failure.

[str4d]

I made a close review pass over the RSASSA-PSS implementation. The relevant sections of RFC 8017 were already inlined as comments.

[str4d]

Is there precedent for detecting the salt length? I know it is possible due to the structure of DB, but there is no mention of doing so in RFC 8017. I can't read the IEEE documents that reference different possible salt lengths, which maybe discusses this. As is, we never exercise the explicit-salt-length case below.

[dignifiedquire]

I don't know @roblabla I believe you wrote this code initially, do you have any answer to this?

[str4d]

Is there precedent for this default salt length? The RFC mentions that common choices are digest.output_size() and 0, while this is selecting the salt to be as large as could fit. I can't read the IEEE documents that reference different possible salt lengths. In any case, I don't think there's a problem with this, given that we are detecting the salt length in verify.

[str4d]

Length is not checked, but with the current API we never actually see the message.

[str4d]

Length is not checked, but with the current API we never actually see the message.

[str4d]

This would be clearer (and closer to the spec):

Suggested change

	let h = &mut h[..(em_len - 1) - (em_len - h_len - 1)];
	let h = &mut h[..h_len];

[dignifiedquire]

fixed

[str4d]

Suggested change

	None => (0..=em_len - (h_len + 2))
	.rev()
	.try_fold(None, |state, i| match(state, db[em_len - h_len - i - 2]){
	None => (0..em_len - h_len - 1)
	.try_fold(None, |state, i| match(state, db[i]){

[dignifiedquire]

this change results in failures, haven't debugged why, gonna leave the logic as is for now

[str4d]

Suggested change

	for e in&db[..em_len - h_len - s_len - 2]{
	if*e != 0x00{
	returnErr(Error::Verification);
	}
	}
	if db[em_len - h_len - s_len - 2] != 0x01{
	returnErr(Error::Verification);
	}
	let(zeroes, rest) = db.split_at(em_len - h_len - s_len - 2);
	if zeroes.iter().any(|e| *e != 0x00) || rest[0] != 0x01{
	returnErr(Error::Verification);
	}

[dignifiedquire]

fixed

[str4d]

Suggested change

	ifInto::<bool>::into(h0.ct_eq(h)){
	if h0.ct_eq(h).into(){

[dignifiedquire]

fixed

[str4d]

Suggested change

	let(db, h) = em.split_at_mut(em_len - s_len - h_len - 2 + 1 + s_len);
	let(db, h) = em.split_at_mut(em_len - h_len - 1);

[dignifiedquire]

fixed

[str4d]

We're assuming here that the em buffer passed into emsa_pss_encode is already zeroed. This happens to be the case, but given that emsa_pss_encode is only called from one place, and this PR isn't attempting to be no-std compatible, it would be clearer if em was constructed and returned from this function. Non-blocking.

[dignifiedquire]

fixed

[dignifiedquire]

This integrates #18 and #26 by expanding the use of PaddingScheme to capture all optional values for the different schemes.

It is not ideal, but it works well and keeps in line with the existing API for now. I think we should still continue to improve on the api and think through #34 more, but in the meantime this a version that folks can start to use, and isn't too different from the existing api

[str4d]

Hmm, pulling in 2462f1d from #18 goes in the opposite direction to #42. It looks like the main thing it does (remove PublicKey trait) is not present in the overall PR, and I can't find where that is undone, but I think it's in the refactor commit? GitHub isn't helping much here. There are other effects of that commit that are not undone (in particular, it makes public keys non-generic in pkcs1v15, the opposite of what I've done in #44).

How painful would it be to just drop that commit entirely? It would likely make the history clearer.

[str4d]

Hmm, the change in that commit to store an RSAPublicKey inside RSAPrivateKey does make sense to me. I'd personally alter that commit to only contain the pieces we want, but that's probably more hassle than it's worth, so just reverting changes we don't want is probably fine.

[dignifiedquire]

Yeah, I changed a lot of things during the merge, which is..unfortunate. So at this point I am thinking of squashing this into two commits at the end likely (1) for oaep and (2) for pss, after doing/undoing all things we want to do.

[dignifiedquire]

@str4d I made the OAEP and PSS impls generic over the Key traits now

[roblabla]

Hey that looks great! I have a couple suggestions

[roblabla]

Could make it Box<dyn DynDigest + Debug> if really necessary. AFAIK all the digest crates have debug unconditionally implemented on them.

[dignifiedquire]

that doesn't work, the compiler is upset about more than one non auto trait :(

[roblabla]

Ah right, it's possible to make your own trait (e.g. trait MyDynDigest: DynDigest + Debug {}) and then use that as a Box<dyn MyDynDigest>, but at this point it's too much boilerplate just for the sake of getting the digest name to show IMO.

[tarcieri]

Relevant to the PSS use case: final two week comment period for shipping signature 1.0: RustCrypto/traits#78

[str4d]

Now that we have PrivateKey: PublicKeyParts, this impl Deref should be removed.

[dignifiedquire]

Not quite, removing this, removes the ability to call .verify on a private key

[str4d]

Should this check also be in raw_decryption_primitive?

[dignifiedquire]

@str4d thanks for the review, I think I addressed all important things

[str4d]

The hash: None case appears to still be necessary for unpadded signatures (at least, that's what the test case in src/pkcs1v15.rs leads me to believe).

[dignifiedquire]

Yes, which is why it is still an Option, not sure what you mean.

[str4d]

I was just noting this because it wasn't immediately obvious during review that the Option<Hash> was necessary besides enabling the prior encryption case to specify None.

[str4d]

I made a close review pass over the RSAES-OAEP implementation, comparing it against RFC 8017 section 7.1.

[dignifiedquire]

@str4d applied the new round of CR

[str4d]

These are causing the current CI failure.

[str4d]

I made a close review pass over the RSASSA-PSS implementation. The relevant sections of RFC 8017 were already inlined as comments.

[str4d]

Is there precedent for detecting the salt length? I know it is possible due to the structure of DB, but there is no mention of doing so in RFC 8017. I can't read the IEEE documents that reference different possible salt lengths, which maybe discusses this. As is, we never exercise the explicit-salt-length case below.

[dignifiedquire]

I don't know @roblabla I believe you wrote this code initially, do you have any answer to this?

[str4d]

Is there precedent for this default salt length? The RFC mentions that common choices are digest.output_size() and 0, while this is selecting the salt to be as large as could fit. I can't read the IEEE documents that reference different possible salt lengths. In any case, I don't think there's a problem with this, given that we are detecting the salt length in verify.

[str4d]

Length is not checked, but with the current API we never actually see the message.

[str4d]

Length is not checked, but with the current API we never actually see the message.

[str4d]

This would be clearer (and closer to the spec):

Suggested change

	let h = &mut h[..(em_len - 1) - (em_len - h_len - 1)];
	let h = &mut h[..h_len];

[dignifiedquire]

fixed

[str4d]

Suggested change

	None => (0..=em_len - (h_len + 2))
	.rev()
	.try_fold(None, |state, i| match(state, db[em_len - h_len - i - 2]){
	None => (0..em_len - h_len - 1)
	.try_fold(None, |state, i| match(state, db[i]){

[dignifiedquire]

this change results in failures, haven't debugged why, gonna leave the logic as is for now

[str4d]

Suggested change

	for e in&db[..em_len - h_len - s_len - 2]{
	if*e != 0x00{
	returnErr(Error::Verification);
	}
	}
	if db[em_len - h_len - s_len - 2] != 0x01{
	returnErr(Error::Verification);
	}
	let(zeroes, rest) = db.split_at(em_len - h_len - s_len - 2);
	if zeroes.iter().any(|e| *e != 0x00) || rest[0] != 0x01{
	returnErr(Error::Verification);
	}

[dignifiedquire]

fixed

[str4d]

Suggested change

	ifInto::<bool>::into(h0.ct_eq(h)){
	if h0.ct_eq(h).into(){

[dignifiedquire]

fixed

[str4d]

Suggested change

	let(db, h) = em.split_at_mut(em_len - s_len - h_len - 2 + 1 + s_len);
	let(db, h) = em.split_at_mut(em_len - h_len - 1);

[dignifiedquire]

fixed

[str4d]

We're assuming here that the em buffer passed into emsa_pss_encode is already zeroed. This happens to be the case, but given that emsa_pss_encode is only called from one place, and this PR isn't attempting to be no-std compatible, it would be clearer if em was constructed and returned from this function. Non-blocking.

[dignifiedquire]

fixed