openssl icon indicating copy to clipboard operation
openssl copied to clipboard

Attribute certificate (RFC 5755)

Open dhobsong opened this issue 4 years ago • 97 comments

Closes #14648

Please see the checklist below for sub-task completion status.

Right now I've split this implementation over several patches to try to keep the sizes a little more manageable, but I can squash them if that makes more sense.

Some of the areas of concern regarding this patch series are listed below. Any feedback related to these (or anything else) would be greatly appreciated

Attribute values

RFC 5755 defines several Attribute types that have ASN.1 SEQUENCEs as their value fields. E.g. group and role. As far as I can tell, there is no nice way to handle Attributes that don't have string (or similar) values for printing or parsing. The current code just uses the output of ANS1_item_print() for printing, and parses from the config file using ASN1_generate_nconf(). Maybe some framework, similar to the X509V3_EXT_METHOD used for extensions might be useful for X509_ATTRIBUTEs?

public API

RFC 5755 defines the attribute certificate format using several new ASN.1 sequence types that don't seem to be used in any other profiles. I've tried to find a good balance of what to expose in the public API, but if there are any opinions on whether I should be exposing more or less, I would be happy to hear them.

file separation

I'm not 100% sure that all of the function implementations are in the right location (correct function in correct file). I've tried to follow the way that the X509, X509_REQ and X509_CRL implementation is divided, but maybe there is some room for improvement there.

Checklist

  • [x] file / BIO input output for attribute certificates (AC)
  • [x] setters / getters for AC fields
  • [x] read AC attribute values from config file
  • [x] documentation is added or updated
  • [x] tests are added or updated

dhobsong avatar Jun 22 '21 07:06 dhobsong

Nice!

Note that this is too late for inclusion in 3.0 and has been added to the "post 3.0" milestone. For that reason its unlikely to get much developer attention until 3.0 is out.

Some of the CI failures seem relevant.

mattcaswell avatar Jun 24 '21 07:06 mattcaswell

Hi @mattcaswell,

Thanks for the info. I'll address the CI failures and push forward for now.

dhobsong avatar Jun 24 '21 08:06 dhobsong

@FdaSilvaYY, thanks for the quick review.
I'll make sure to add in the missing oom error blocks and address the nits and make sure there are no other instances of the same (there probably are). I switched my compiler to clang from gcc, which highlighted one or two more warnings, so I'll address those too.

Any comments about how to print X509_ATTRIBUTEs in a generic way? Maybe I should open a separate issue for that.

dhobsong avatar Jun 24 '21 10:06 dhobsong

v2 changes

  • Addressed the review comments from @FdaSilvaYY (thank you again for your feedback)
  • Changed the argument / return type of X509_ACERT_get0/set0_holder_entityName() from X509_NAME to GENERAL_NAMES
    • reason: entity name needs to be able to match against a value the SAN in the holder certificate, which can contain types other than X509_NAME
  • Moved all getter/setter function definitions from x509.h.in to x509v3.h.in
    • reason: X509_ACERT_get0/set0_holder_entityName() needs the definition of GENERAL_NAME which is only defined in x509v3.h.in. Other functions may relay on other "V3 only" definitions in future.

dhobsong avatar Jul 05 '21 06:07 dhobsong

v3 changes

  • Addressed previous review comments
  • Make sure getter / setter functions that handle pointer types use the get0/get1 set0/set1 convention
  • Added attribute certificate serial number to the output of X509_ACERT_print_{ex} functions
  • renamed IETF_ATTR_SYNTAX_print_ex() to IETF_ATTR_SYNTAX_print() as no previous version exists

dhobsong avatar Jul 15 '21 09:07 dhobsong

@dhobsong Can I ask you what your expectations are for the scope of your attribute certificates implementation? Are you expecting to implement:

  • Signature verification?
  • Validity checking?
  • holder matching?
  • ACRL checking?
  • OCSP checking?
  • Any particular extensions?
  • Any particular attributes?
  • Command-line usage?

Thank you for starting this!

JonathanWilbur avatar Jul 16 '21 11:07 JonathanWilbur

Hi @JonathanWilbur I was looking to implement

  • Signature verification
  • Validity checking (i.e. the checks from Section 5 of the RFC)
  • holder matching (if you mean verifying that a given attribute certificate corresponds to a given PKI certificate)
  • Command-line usage

The current patch set implements the IETFAttrSyntax, but I may also use some others (not yet decided). As I mentioned in the top of this PR though, some more work may be needed to better handle setting/parsing before adding more new attributes.

TBH I hadn't really give OCSP, ACRL and additional extensions much thought so far.

All of this is dependent on this PR of course though.

dhobsong avatar Jul 19 '21 10:07 dhobsong

v4 changes

  • Added / updated documentation
  • made X509_ACERT_get0_issuerName() and X509_ACERT_get0_info_signature() return non-const values to make them similar to the X509 versions
  • Added X509_ACERT_get_signature_nid() to be consistent with the signature API for X509, X509_CRL, etc.

The CI test suite should now pass all tests (previously was failing doc-nits), so I will remove the draft status. Comments are still very welcome.

dhobsong avatar Jul 30 '21 10:07 dhobsong

v5 changes

  • Rebased to master
  • Addressed previous documentation comments

dhobsong avatar Aug 26 '21 02:08 dhobsong

rebase and re-push

dhobsong avatar Nov 25 '21 06:11 dhobsong

I see that this task is in OMC review. I know a lot of people don't know about attribute certificates and their significance. I'd be happy to speak to the importance of attribute certificates if anybody in the OMC feels they could use some guidance on what attribute certificates are and how attribute certificates are used.

JonathanWilbur avatar Jan 07 '22 03:01 JonathanWilbur

Post a summary of your views here. That helps everyone see the context you are focused on.

t-j-h avatar Jan 07 '22 03:01 t-j-h

X.509 certificates got their name because they originate from ITU Recommendation X.509 (not IETF RFC 5280 or its precursors). But this is not the only thing specified in ITU Recommendation X.509. This standard also defines attribute certificates, which function more like a JSON Web Token: they are a general-purpose digitally authenticated collection of attributes (although their intended use was primarily privilege management). The equivalent of the public-key certificates subject field in an attribute certificate is the holder field, which relates the group of attributes to a public-key certificate, to an X.500 directory name, or something else.

Several large companies (I am under a non-disclosure agreement, but you'd recognize their names) are interested in seeing attribute certificate support incorporated into OpenSSL. At least one such sought-after use case I know of is Platform Certificates (a subtype of an attribute certificate), which provide supply chain integrity for hardware. I'd be happy to delve into further detail, but here are some links pertaining to platform certificates and attribute certificates that might explain this better than I could.

  • https://www.youtube.com/watch?v=Yh-OTPXBaps
  • https://www.youtube.com/watch?v=muKP_2NMSFo
  • https://www.nccoe.nist.gov/supply-chain-assurance
  • https://en.wikipedia.org/wiki/Authorization_certificate

JonathanWilbur avatar Jan 10 '22 11:01 JonathanWilbur

Thanks @JonathanWilbur for advocating for this, and for adding relevant reference material.

Just to add a little bit of context, my particular use case is for privilege management, which is why this patch series includes support for the IETFAttrSyntax type, which is defined in the spec for representing group membership of the attribute certificate holder.

I've rebased the patch series again. The CI tests all pass on my local branch. The last time I pushed, it seemed like there was and issue with AVX512 support unrelated to this patch series.

dhobsong avatar Jan 11 '22 09:01 dhobsong

Thank you @JonathanWilbur for advocating for this, Actually I am working with @dhobsong.

RFC 5755 is used TCG Platform Certificate Profile. However, as far as I know, there is only one software that supports the specification, provided by the NSA.

https://github.com/nsacyber/paccor

There is a lot of talk about the importance of the supply chain(i.e SP 800-147, SP 800-193), but support tools is limited.

RFC 5755 is to make it possible for OpenSSL, which is used in a variety of environments, users can use OpenSSL to verify the firmware of their devices.

tatac1 avatar Jan 11 '22 15:01 tatac1

+1 on this feature. I've hacked around TCG Platform Certificates in the past but it was no fun due to lack of broader support. Platform Certs as such are a great concept but suffer from chicken-egg problem. So adoption by OpenSSL as major software library would be a great enabler. Also from the tpm2-software/tpm2-tss standpoint we have the platform cert retrieval functions in place.

(Full disclosure: I'm also a TCG member; focused on the TPM Software Stack)

AndreasFuchsTPM avatar Feb 03 '22 16:02 AndreasFuchsTPM

+1 on he feature, I work on the tpm2-software org here on github, and it would be nice to have this in place in OSSL.

williamcroberts avatar Feb 03 '22 16:02 williamcroberts

This PR is in a state where it requires action by @openssl/omc but the last update was 39 days ago

openssl-machine avatar Mar 15 '22 00:03 openssl-machine

This PR is in a state where it requires action by @openssl/omc but the last update was 70 days ago

openssl-machine avatar Apr 15 '22 00:04 openssl-machine

This PR is in a state where it requires action by @openssl/omc but the last update was 101 days ago

openssl-machine avatar May 16 '22 00:05 openssl-machine

OMC have voted to lift the OMC hold on this PR.

mattcaswell avatar Jun 01 '22 10:06 mattcaswell

You will need to rebase the PR to the fresh master branch to resolve conflicts.

t-j-h avatar Jun 27 '22 00:06 t-j-h

You will need to rebase the PR to the fresh master branch to resolve conflicts.

@t-j-h, thanks for the heads up. I'm moving house this week, but I think that I should be able to rebase and retest this late next week, or the week following.

dhobsong avatar Jul 01 '22 02:07 dhobsong

I have rebased the patch set to the current top of the master branch and resolved all outstanding conflicts.

dhobsong avatar Jul 18 '22 13:07 dhobsong

This PR is in a state where it requires action by @openssl/otc but the last update was 30 days ago

openssl-machine avatar Aug 18 '22 00:08 openssl-machine

@FdaSilvaYY and @iqbal0kabat , is there any reason this couldn't be merged in now? I have another pull request outstanding that would provide no added functionality unless this were merged in.

JonathanWilbur avatar Sep 06 '22 23:09 JonathanWilbur

This PR needs to be reviewed and approved by @openssl members. Subject to any merge conflict already resolved. I'm just trying to reduce the team burden by reviewing a bit some changes, when I have some spare time.

FdaSilvaYY avatar Sep 07 '22 09:09 FdaSilvaYY

OTC: Are the newly added public identifiers OK, or do they have to be prefixed with OSSL_/ossl_ prefix?

t8m avatar Sep 07 '22 09:09 t8m

IIRC, the OSSL_/ossl_ prefix policy applies strictly only to new 'namespaces', not to existing ones like 'x509'.

In the case of x509.h it would be quite inconsistent to start adding those prefixes.

mspncp avatar Sep 07 '22 14:09 mspncp

@mspncp please note there are identifiers like: OBJECT_DIGEST_INFO, ISSUER_SERIAL, IETF_ATTR_SYNTAX being added.

t8m avatar Sep 08 '22 05:09 t8m