Skip to content

Adapt ASN1_item_sign_ctx() for use with provided keypairs#10920

Closed
levitte wants to merge 10 commits intoopenssl:masterfrom
levitte:prov-algorithmidentifier
Closed

Adapt ASN1_item_sign_ctx() for use with provided keypairs#10920
levitte wants to merge 10 commits intoopenssl:masterfrom
levitte:prov-algorithmidentifier

Conversation

@levitte
Copy link
Member

@levitte levitte commented Jan 21, 2020

The mechanism to do this is to ask the signature operation for the DER
encoded AlgorithmIdentifier that corresponds to the combination of
signature algorithm and digest algorithm.

The provided DSA implementation is adapted to suit.

@levitte levitte added branch: master Applies to master branch approval: review pending This pull request needs review by a committer labels Jan 21, 2020
@levitte
Copy link
Member Author

levitte commented Jan 21, 2020

The EXPERIMENTAL (#10797) test failure that triggered this is test_req

@mattcaswell
Copy link
Member

Travis failure appears relevant.

@levitte
Copy link
Member Author

levitte commented Jan 22, 2020

There will be corresponding work for ASN1_item_verify_ctx. That's a bit more involved because of horrid SM2 hackery in x_all.c. I'm pondering if I should make that a separate PR or include that in this PR. Thoughts?

@levitte
Copy link
Member Author

levitte commented Jan 22, 2020

Travis failure appears relevant.

It should be fine now

@slontis
Copy link
Member

slontis commented Jan 23, 2020

It should be fine now

Rebuilt the failing travis tests (now that the error in master is fixed).
This should now only show real errors

@levitte levitte force-pushed the prov-algorithmidentifier branch from 6e0f879 to 4b10f1d Compare January 23, 2020 17:36
@slontis
Copy link
Member

slontis commented Jan 23, 2020

There is a memory leak..

@paulidale
Copy link
Contributor

Is Travis relevant here? (fuzz test failure).

@slontis
Copy link
Member

slontis commented Jan 24, 2020

Is Travis relevant here? (fuzz test failure).

Its a memory leak picked up by asan

@levitte
Copy link
Member Author

levitte commented Jan 24, 2020

Time for a re-review

@levitte levitte force-pushed the prov-algorithmidentifier branch from 1b593bc to 71ee5da Compare January 25, 2020 03:47
@levitte levitte force-pushed the prov-algorithmidentifier branch from ec5fd73 to 62e2474 Compare January 26, 2020 16:23
@slontis slontis added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Jan 26, 2020
openssl-machine pushed a commit that referenced this pull request Jan 28, 2020
The mechanism to do this is to ask the signature operation for the DER
encoded AlgorithmIdentifier that corresponds to the combination of
signature algorithm and digest algorithm.

Reviewed-by: Shane Lontis <[email protected]>
(Merged from #10920)
openssl-machine pushed a commit that referenced this pull request Jan 28, 2020
openssl-machine pushed a commit that referenced this pull request Jan 28, 2020
@levitte
Copy link
Member Author

levitte commented Jan 28, 2020

Merged

d5aef59 Adapt ASN1_item_sign_ctx() for use with provided keypairs
505b41f PROV: Adapt the DSA signature implementation to provide Algorithmidentifiers
0cb3f4f test_evp_extra_test.c: don't rely on exact parameter position

@levitte levitte closed this Jan 28, 2020
@slontis slontis mentioned this pull request Jan 29, 2020
2 tasks
ENCODE_ALGORITHMIDENTIFIER_SHAx(sha3_224, 5);
ENCODE_ALGORITHMIDENTIFIER_SHAx(sha3_256, 6);
ENCODE_ALGORITHMIDENTIFIER_SHAx(sha3_384, 7);
ENCODE_ALGORITHMIDENTIFIER_SHAx(sha3_512, 8);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is perfect for FIPS. It isn't so good if dsa-with-XXX exists for other digests. Probably more of an issue for ECDSA than here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approval: done This pull request has the required number of approvals branch: master Applies to master branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants