Skip to content

Add X509_cmp_timeframe()#10502

Closed
DDvO wants to merge 5 commits intoopenssl:masterfrom
siemens:add_X509_cmp_timeframe
Closed

Add X509_cmp_timeframe()#10502
DDvO wants to merge 5 commits intoopenssl:masterfrom
siemens:add_X509_cmp_timeframe

Conversation

@DDvO
Copy link
Contributor

@DDvO DDvO commented Nov 22, 2019

This PR adds the function X509_cmp_timeframe() along with documentation and thorough tests.
It also fixes a parameter renaming glitch in the current documentation of X509_cmp_time().

The function can be used for instance as follows:

    X509_VERIFY_PARAM *vpm = ... /* may include flags and reference time used below */
    X509 *cert = ...
    int time_cmp = OSSL_CMP_cmp_timeframe(vpm,
                                          X509_get0_notBefore(cert),
                                          X509_get0_notAfter(cert));
    if (time_cmp > 0)
        error("certificate expired");
    if (time_cmp < 0)
        error("certificate not yet valid");

The motivation for adding this function to x509_vfy.c is that it is currently defined in a preview of the upcoming CMP contribution chunk 7 while it could be of broader use, as discussed with @mattcaswell here: mpeylo#199 (comment)

  • documentation is added or updated
  • tests are added or updated

@DDvO DDvO mentioned this pull request Nov 22, 2019
Closed
2 tasks
mattcaswell
mattcaswell requested changes Nov 22, 2019
View reviewed changes
@mattcaswell mattcaswell added the branch: master Applies to master branch label Nov 22, 2019
@DDvO
Copy link
Contributor Author

DDvO commented Nov 22, 2019 •
edited
Loading

The CI tests pointed out a const glitch, which I solved essentially by constifying X509_VERIFY_PARAM_get_flags() (which was still missing anyway).

mattcaswell
mattcaswell reviewed Nov 22, 2019
View reviewed changes
mattcaswell
mattcaswell approved these changes Nov 22, 2019
View reviewed changes
Copy link
Member

@mattcaswell mattcaswell left a comment

Choose a reason for hiding this comment

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

I'm tentatively approving this, subject to input from others about the naming issue.

@mattcaswell mattcaswell added the approval: review pending This pull request needs review by a committer label Nov 22, 2019
@richsalz
Copy link
Contributor

richsalz commented Nov 22, 2019

FWIW, when I saw the function name, I thought it would take an X509 cert, not a VPM.

@DDvO DDvO mentioned this pull request Nov 22, 2019
Closed
1 task
@mattcaswell
Copy link
Member

mattcaswell commented Nov 27, 2019

Ping - needs second review

paulidale
paulidale approved these changes Nov 27, 2019
View reviewed changes
Copy link
Contributor

@paulidale paulidale left a comment

Choose a reason for hiding this comment

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

A suggested wording change which I think improves things but others might not.
Approved either way.

@DDvO
Copy link
Contributor Author

DDvO commented Nov 28, 2019

FWIW, when I saw the function name, I thought it would take an X509 cert, not a VPM.

Concerning the function name, right that the X509_ prefix should indicate that the function takes a cert argument (as its first parameter). Yet this also applies to all the the other time-related functions defined in x509_vfy.c, and the new function just follows the naming model of its peers.

One could consider consistently renaming all these functions, including the new one, to something that does not contain X509_ and move the functions out of crypto/x509/ but this would be the topic of a separate PR.

@DDvO
Copy link
Contributor Author

DDvO commented Nov 28, 2019

Can this PR be flagged for merging now?

@levitte levitte 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 Nov 28, 2019
@levitte
Copy link
Member

levitte commented Nov 28, 2019

Can this PR be flagged for merging now?

The conclusive approval was done 16 hours ago. So, with regards to 24h grace period that we have regimented ourselves to, it can in 8 hours.

@paulidale paulidale added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Dec 3, 2019
@DDvO DDvO force-pushed the add_X509_cmp_timeframe branch from 80d61fa to ebd4db6 Compare December 4, 2019 12:08
@DDvO DDvO force-pushed the add_X509_cmp_timeframe branch from ebd4db6 to 2bf6fb7 Compare December 4, 2019 12:50
@DDvO
Copy link
Contributor Author

DDvO commented Dec 4, 2019

In the meantime I had to rebase this (fixing a minor merge conflict in libcrypto.num).

Could someone please merge this PR.

openssl-machine pushed a commit that referenced this pull request Dec 4, 2019
@DDvO @mattcaswell
add X509_cmp_timeframe() including its documentation
25d7cd1 
Reviewed-by: Paul Dale <[email protected]>
Reviewed-by: Matt Caswell <[email protected]>
(Merged from #10502)
@mattcaswell
Copy link
Member

mattcaswell commented Dec 4, 2019

I squashed the various commits and merged this. Closing.

@mattcaswell mattcaswell closed this Dec 4, 2019
@DDvO DDvO deleted the add_X509_cmp_timeframe branch December 6, 2019 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@levitte levitte levitte left review comments

@mattcaswell mattcaswell mattcaswell approved these changes

@paulidale paulidale paulidale approved these changes

Assignees

No one assigned

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Applies to master branch

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@DDvO @richsalz @mattcaswell @levitte @paulidale