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

Add support for Certificate Revocation List files. #52

Merged
merged 1 commit into from
Jul 8, 2021

Conversation

clalancette
Copy link
Contributor

@clalancette clalancette commented Jun 28, 2021

At least Fast-RTPS and CycloneDDS support Certificate
Revocation Lists, so add it as one of the possible files
in the enclave. Note that it is an optional file; if
an enclave doesn't have it, then the key will be missing
from the returned map.

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

Besides the maintainers of this package, also pinging @ruffsl , @mikaelarguedas , and @SidFaber . The addition of this file is essentially expanding the on-disk "API" of an SROS2 security enclave to include the crl.pem file, so I'd appreciate any feedback that you may have.

@clalancette
Copy link
Contributor Author

clalancette commented Jun 30, 2021

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@ruffsl
Copy link
Member

ruffsl commented Jun 30, 2021

From the secure DDS spec:

If the property ocsp_status is present, the operation shall verify that the OCSP
response included in the property corresponds to the identity in the c.id property.
The operation shall use the OCSP response to verify the status of the
IdentityCredential. If that status is good and the validity interval has not
been exceeded it shall accept that as proof that the IdentityCredential is still valid.
If the status is revoked, the operation shall fail and return ValidationResult_Fail. If
the status is different from the aforementioned ones it shall behave as if the
ocsp_status property was not present.
If the property ocsp_status is not present, the operation shall use its own means to
determine the status of the IdentityCredential. This may performing an
OCSP query or consulting a CRL list. The specific behavior is implementation
specific.

Is there a consensus among the DDS vendors on how the CRL list is to be encoded, or how enabling it's configuration as implemented?

@clalancette
Copy link
Contributor Author

Is there a consensus among the DDS vendors on how the CRL list is to be encoded, or how enabling it's configuration as implemented?

For the encoding, both open-source implementations (CycloneDDS and Fast-DDS) rely on OpenSSL (and more specifically, X509_STORE_add_crl) to make CRLs work. That means that they both expect the CRL to be of the form:

-----BEGIN X509 CRL-----
MIHTMHoCAQEwCgYIKoZIzj0EAwIwEjEQMA4GA1UEAwwHc3JvczJDQRcNMjEwNjA3
...
-----END X509 CRL-----

(it is unclear whether or how Connext implements CRLs; maybe @asorbini can comment?)

As far as how to enable CRLs, that is done differently per DDS implementation:

  • For CycloneDDS, the CRL is specified by setting org.eclipse.cyclonedds.sec.auth.crl on the property QoS. Alternatively, CycloneDDS also supports setting it in a configuration file during startup.
  • For Fast-DDS, the CRL is specified by setting dds.sec.auth.builtin.PKI-DH.identity_crl on the property QoS. Alternatively, I believe Fast-DDS also supports setting it in a configuration file during startup.

At least Fast-RTPS and CycloneDDS support Certificate
Revocation Lists, so add it as one of the possible files
in the enclave.  Note that it is an optional file; if
an enclave doesn't have it, then the key will be missing
from the returned map.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette clalancette force-pushed the clalancette/add-crl branch from 3c374f2 to 6d7df2b Compare July 6, 2021 15:40
@clalancette
Copy link
Contributor Author

clalancette commented Jul 6, 2021

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@clalancette
Copy link
Contributor Author

CI is green, and this has been approved. I'm going to go ahead and merge this now; if there are any further questions about how this fits into an enclave, we can do follow-ups to make changes.

@clalancette clalancette merged commit d55fcbc into master Jul 8, 2021
@clalancette clalancette deleted the clalancette/add-crl branch July 8, 2021 13:15
@asorbini
Copy link
Contributor

(it is unclear whether or how Connext implements CRLs; maybe @asorbini can comment?)

@clalancette apologies for the late reply. Connext has supported CRLs for several versions, although I'm not sure about 5.3.1.

I also don't recall the exact CRL format, but I suspect it is the same as other implementation, since our default security plugins also rely on OpenSSL. The documentation for the authentication.crl mentions "PEM format", which is I think what you are referring to.

You can find some more information about CRL support here

I'll look for an authoritative answer on the exact version and format and get back to you.

@clalancette
Copy link
Contributor Author

I'll look for an authoritative answer on the exact version and format and get back to you.

Perfect, thanks @asorbini !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants