Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Chunk 11 of CMP contribution to OpenSSL: CMP command-line interface #11470

Closed
wants to merge 7 commits into from

Conversation

DDvO
Copy link
Contributor

@DDvO DDvO commented Apr 3, 2020

This PR adds the CMP app and its documentation.
CLI-based tests will be the topic of the final chunk 12.

Update: List of open spin-off PRs (including current state) on which this PR has been depending:

The CLI enables everyone to easily try out typical certificate management use cases
with the demo configuration added to apps/openssl.cnf, which refers to the Insta Demo CA.
For instance, as described in the EXAMPLES section of doc/man1/openssl-cmp.pod:

  export OPENSSL_CONF=apps/openssl.cnf
  export LD_LIBRARY_PATH=.
  wget 'http://pki.certificate.fi:8080/install-ca-cert.html/ca-certificate.crt\
        ?ca-id=632&download-certificate=1' -O insta.ca.crt
  openssl genrsa -out insta.priv.pem

  apps/openssl cmp -section insta,ir                 # initial cert request
  apps/openssl x509 -noout -text -in insta.cert.pem  # view the enrolled cert

  apps/openssl cmp -section insta,cr                 # subsequent cert request
  apps/openssl cmp -section insta,kur                # cert update
  apps/openssl cmp -section insta,rr,signature       # cert revocation

@DDvO
Copy link
Contributor Author

DDvO commented Apr 3, 2020

This is a pretty large body of code and documentation,
mostly due to the flexibility of CMP and the many options implemented.

@DDvO
Copy link
Contributor Author

DDvO commented Apr 3, 2020

About 1000 of the 3500 lines of code in apps/cmp.c are of more generic nature,
for example implementing more flexible and convenient loading of credentials etc.
Most of those functions could be moved to apps/lib/, as indicated by the TODOs given.

I had tried contributing such changes long ago in the PRs #4277, #4930, and #4940,
but that got stuck due to lack of general interest.
I could revive that as part of the present PR, but I don't think it's strictly needed.
Let's see what comments will come up on this topic.

@DDvO
Copy link
Contributor Author

DDvO commented Apr 4, 2020

Travis CI took too long on one of the test runs as usual; everything else went fine.
BTW, how about increasing that 50 mins timeout value to, e.g., 60 mins?

@richsalz
Copy link
Contributor

richsalz commented Apr 4, 2020

It already is 60 minutes. @t8m is working on a PR to remove that build

@DDvO
Copy link
Contributor Author

DDvO commented Apr 7, 2020

Rebased to the latest master (now including the fixes of #11448).

@DDvO
Copy link
Contributor Author

DDvO commented Apr 7, 2020

Has anyone already tried out the CMP app? For instance:

OPENSSL_CONF=apps/openssl.cnf apps/openssl cmp -section insta

@DDvO
Copy link
Contributor Author

DDvO commented Apr 14, 2020

Currently shown Travis CI issues are unrelated -
still the usual timeout on one build, and this time also some krb5 test failure.
Rebasing...

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.

Some review comments so far. I have not yet looked at apps/cmp.c - but I've looked at everything else.

apps/openssl-vms.cnf Outdated Show resolved Hide resolved
apps/openssl.cnf Outdated Show resolved Hide resolved
doc/man1/openssl-cmp.pod.in Outdated Show resolved Hide resolved
doc/man1/openssl-cmp.pod.in Outdated Show resolved Hide resolved
doc/man1/openssl-cmp.pod.in Outdated Show resolved Hide resolved
doc/man1/openssl-cmp.pod.in Show resolved Hide resolved
doc/man1/openssl-cmp.pod.in Show resolved Hide resolved
doc/man1/openssl-cmp.pod.in Outdated Show resolved Hide resolved
doc/man1/openssl-cmp.pod.in Outdated Show resolved Hide resolved
doc/man1/openssl-cmp.pod.in Outdated Show resolved Hide resolved
@DDvO
Copy link
Contributor Author

DDvO commented Apr 14, 2020

Some review comments so far. I have not yet looked at apps/cmp.c - but I've looked at everything else.

Thanks @mattcaswell for these comments - I've answered/handled all of them.
Makes much sense to start by having a look at the documentation first.

@DDvO
Copy link
Contributor Author

DDvO commented Apr 15, 2020

A bit awkward that changes to apps/openssl.cnf must be kept in sync with apps/openssl-vms.cnf.
I just found that this is done by make generate_apps, as part of make update.

apps/cmp.c Outdated Show resolved Hide resolved
apps/cmp.c Outdated Show resolved Hide resolved
apps/cmp.c Outdated Show resolved Hide resolved
@levitte
Copy link
Member

levitte commented Apr 16, 2020

I had tried contributing such changes long ago in the PRs #4277, #4930, and #4940, but that got stuck due to lack of general interest.

I'll regard that as a huge PING!
It would be better to review those separately, rather that have them gobbled up in this PR

@DDvO
Copy link
Contributor Author

DDvO commented Apr 20, 2020

I'll regard that as a huge PING!
It would be better to review those separately, rather that have them gobbled up in this PR

@levitte I'm glad that you picked this up.
As I wrote above the present PR also works without those three PRs, but it's going to become a bit shorter and cleaner (with the respective TODOs I included in apps/cmp.c having been resolved).
So I've dusted off those PRs (i.e.., rebased them etc.) and started handling your new comments there.

@mattcaswell, this consolidation activity should not have much effect on your review of apps/cmp.c in the sense that there is enough other code to look at.
I wonder if you already started reviewing this file?

@mattcaswell
Copy link
Member

I wonder if you already started reviewing this file?

I started to look and then asked @levitte if he could have a look at the engine related stuff. I will try to get back to it...but I'm a little distracted by the looming alpha1 at the moment. Probably it will be later the week before I can look.

@DDvO
Copy link
Contributor Author

DDvO commented Apr 24, 2020

Rebased on current master, which includes the newly merged #4940.

@DDvO DDvO closed this Apr 28, 2020
@DDvO DDvO reopened this Apr 28, 2020
apps/cmp.c Show resolved Hide resolved
apps/cmp.c Outdated Show resolved Hide resolved
apps/cmp.c Outdated Show resolved Hide resolved
apps/cmp.c Outdated Show resolved Hide resolved
apps/cmp.c Outdated Show resolved Hide resolved
apps/cmp.c Outdated Show resolved Hide resolved
apps/cmp.c Outdated Show resolved Hide resolved
apps/cmp.c Outdated Show resolved Hide resolved
apps/cmp.c Outdated Show resolved Hide resolved
apps/cmp.c Outdated Show resolved Hide resolved
@DDvO
Copy link
Contributor Author

DDvO commented May 12, 2020

@slontis, let's do the adaptation of the CMP & CRMF lib modules for providers in a separate PR.
When this PR #11470 been merged we can rebase on it and adapt also the CLI as far as needed.

Who should be the one setting up that PR and maintaining the branch with the code changes?
I think I can start contributing to this topic tomorrow.

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.

LGTM

@mattcaswell mattcaswell added approval: done This pull request has the required number of approvals branch: master Merge to master branch labels May 12, 2020
@slontis
Copy link
Member

slontis commented May 13, 2020

Who should be the one setting up that PR and maintaining the branch with the code changes?
I think I can start contributing to this topic tomorrow.

I am happy for you to be in charge. Talking to matt he thought that it should be approached from the view of changing the tests to use providers and then get that working.. I can start looking at the test setup side of it if you like.

@DDvO
Copy link
Contributor Author

DDvO commented May 13, 2020

@slontis, sounds good.
So I'm going to set up today a WIP branch starting from changes to the lib components, while you will focus on the test aspects.
Can we somehow work on the same branch (on which repo?), or would you have test-oriented branch based on the former?

@slontis
Copy link
Member

slontis commented May 13, 2020

Can we somehow work on the same branch (on which repo?), or would you have test-oriented branch based on the former?

Just set up a branch like you normally would on master and I will add some commits to it
Should be no problem if we just say it is a WIP.
force pushing is a bit deadly though :)

@DDvO
Copy link
Contributor Author

DDvO commented May 13, 2020

Normally I set up branches on the Siemens repo or the one by @mpeylo where I have write access, but you won't be able to push to either of those, and I since I'm not the owner I cannot give others write access. Is there a repo where we both can push to?

@DDvO
Copy link
Contributor Author

DDvO commented May 13, 2020

Or is it sufficient if you have read access to my WIP branch?

@slontis
Copy link
Member

slontis commented May 13, 2020

See if you can do

>git fetch git@github.com:slontis/openssl.git  cmp_provider_support:{yourbranchname}

@DDvO
Copy link
Contributor Author

DDvO commented May 13, 2020

This did not work directly, but as follows:

git clone git@github.com:slontis/openssl.git slontis
cd slontis
git fetch origin cmp_provider_support:cmp_provider_libext

@slontis
Copy link
Member

slontis commented May 13, 2020

I will stop polluting this PR with comments. We can pollute the new PR instead :)

@DDvO DDvO mentioned this pull request May 13, 2020
2 tasks
@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@mattcaswell mattcaswell 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 May 13, 2020
@mattcaswell
Copy link
Member

Looks like this is good to go @DDvO

openssl-machine pushed a commit that referenced this pull request May 13, 2020
…l.pod

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from #11470)
openssl-machine pushed a commit that referenced this pull request May 13, 2020
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from #11470)
openssl-machine pushed a commit that referenced this pull request May 13, 2020
…#11733

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from #11470)
openssl-machine pushed a commit that referenced this pull request May 13, 2020
Certificate Management Protocol (CMP, RFC 4210) extension to OpenSSL
Also includes CRMF (RFC 4211) and HTTP transfer (RFC 6712).
Adds the CMP and CRMF API to libcrypto and the "cmp" app to the CLI.
Adds extensive documentation and tests.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from #11470)
openssl-machine pushed a commit that referenced this pull request May 13, 2020
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from #11470)
openssl-machine pushed a commit that referenced this pull request May 13, 2020
…actionID()

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from #11470)
openssl-machine pushed a commit that referenced this pull request May 13, 2020
Also update documentation and example code in openssl-cmp.pod.in

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from #11470)
@DDvO
Copy link
Contributor Author

DDvO commented May 13, 2020

Pushed - hooray!
Thanks a lot @mattcaswell!

@DDvO DDvO closed this May 13, 2020
@yangyangtiantianlonglong
Copy link
Contributor

yangyangtiantianlonglong commented May 14, 2020

@DDvO Hi, When will the cmp and crmf feature plans be fully integrated into the openssl master ? I want to make my openssl version plan based on your plan. https://github.com/mpeylo/cmpossl/wiki, I see that the ossl-cmp branch plan is already inaccurate, I hope you can update the plan

@DDvO
Copy link
Contributor Author

DDvO commented May 14, 2020

Hi @yangyangtiantianlonglong,

When will the cmp and crmf feature plans be fully integrated into the openssl master I want to make my openssl version plan based on your plan. https://github.com/mpeylo/cmpossl/wiki, I see that the ossl-cmp branch plan is already inaccurate, I hope you can update the plan

I updated the plan on https://github.com/mpeylo/cmpossl/wiki yesterday, so it is up-to-date.

As of yesterday, the code of the CMP implementation in OpenSSL is complete.
Yet we have some test extensions and various (mostly internal) improvements in the pipeline.
I presume that most of this will be included in the OpenSSL dev master around end of May.

Over the next months I do not expect CMP API changes (and if so, they will be minimal).
The official release of OpenSSL 3.0, including CMP, is planned for October, as you can see here:
https://www.openssl.org/policies/releasestrat.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants