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

Helpers for accessing AKI/SKI extensions of certs/crls #260

Merged
merged 2 commits into from
Sep 27, 2019
Merged

Helpers for accessing AKI/SKI extensions of certs/crls #260

merged 2 commits into from
Sep 27, 2019

Conversation

btoews
Copy link
Contributor

@btoews btoews commented Jun 26, 2019

I'm taking a stab at implementing some of the functionality discussed in #234 (comment). This stuff is kind of tedious to write, so I thought I'd open a PR that just adds helpers for two extensions to make sure we're on the same page about how this should work. Support for more extensions can be added in future PRs if this works out.

This PR adds a #subject_key_identifier method to the Certificate class that parses out the value of the subjectKeyIdentifier extension. This is fairly straightforward. This PR also adds a #authority_key_identifier method to the Certificate and CRL classes that parses out the value of the authorityKeyIdentifier extension. This extension has a number of possible fields and I decided that a Hash was the simplest way to represent this data.

In both cases, I tried to validate the extension contents per the specification and covered many of the edge cases. The flexibility of these extensions makes things difficult though. For example, the authority_cert_issuer field of the authorityKeyIdentifier extension is a GeneralNames, which can be a SEQUENCE of GeneralNames, each of which can be one of nine different types, some of which have multiple possible subtypes. In practice though, this field should always be an RDN sequence. I opted to only handle the common case and return nil otherwise. I'm happy to discuss other options here and to be more or less flexible with these complex types.

/cc @ioquatix

#
# Returns a Hash with keys :key_identifier, :authority_cert_issuer,
# and :authority_cert_serial_number.
def authority_key_identifier
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at Go's implementation, they only expose the key_identifier part of this extension. That seems like a reasonable choice, given that it's what is mostly useful for matching certificates to their issuers. Going that route would also simplify this method here.

@ioquatix
Copy link
Member

I think it's okay to handle a subset of the available data as long as the design doesn't preclude us from supporting more in the future and that it would still make sense given the design choices made.

I would like to avoid methods that contain get, e.g. get_subject_key_id. It's not canonical Ruby. Also try to avoid abbreviations where possible.

@ioquatix ioquatix requested a review from rhenium June 26, 2019 23:08
@ioquatix ioquatix self-assigned this Jun 26, 2019
@ioquatix ioquatix self-requested a review June 26, 2019 23:09
raise ASN1::ASN1Error "invalid GeneralName"
end

if gn_asn1.tag == 4
Copy link
Member

Choose a reason for hiding this comment

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

What is 4?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case, it designates that this is a directoryName (see RFC 5280 Appendix A.2), which is a Name (see RFC 5280 Section 4.1.2.4)

GeneralName ::= CHOICE {
     otherName                 [0]  AnotherName,
     rfc822Name                [1]  IA5String,
     dNSName                   [2]  IA5String,
     x400Address               [3]  ORAddress,
     directoryName             [4]  Name,
     ediPartyName              [5]  EDIPartyName,
     uniformResourceIdentifier [6]  IA5String,
     iPAddress                 [7]  OCTET STRING,
     registeredID              [8]  OBJECT IDENTIFIER }

Name ::= CHOICE { -- only one possibility for now --
     rdnSequence  RDNSequence }

RDNSequence ::= SEQUENCE OF RelativeDistinguishedName

RelativeDistinguishedName ::= SET SIZE (1..MAX) OF AttributeTypeAndValue

AttributeTypeAndValue ::= SEQUENCE {
     type     AttributeType,
     value    AttributeValue }

AttributeType ::= OBJECT IDENTIFIER

AttributeValue ::= ANY -- DEFINED BY AttributeType

Copy link
Member

Choose a reason for hiding this comment

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

Do you think it makes sense to add some constants?

fields[:authority_cert_issuer] = if !issuer.nil?
# TODO raise if there are more than one values? GeneralNames is a
# SEQUENCE.
x509_name_from_general_name(issuer.value.first)
Copy link
Member

Choose a reason for hiding this comment

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

Another option is to return an array, even if it only contains one thing.

@@ -111,13 +111,18 @@ def issue_crl(revoke_info, serial, lastup, nextup, extensions,
crl
end

def get_subject_key_id(cert)
def get_subject_key_id(cert, hex: true)
Copy link
Member

Choose a reason for hiding this comment

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

unpack_subject_key_id might be better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm just updating the existing method here. You'd like me to change the name?

Copy link
Member

Choose a reason for hiding this comment

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

I feel like get_subject_key_id is not a great name, but I'll leave it up to you. It's a relatively minor point.

else
# TODO: raise error instead of returning nil?
# this is some other kind of general name.
nil
Copy link
Member

Choose a reason for hiding this comment

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

I think it's exception: It's not empty, but it's also exception to expected value. Even if our "expected value" is pretty limited at this time.

@btoews
Copy link
Contributor Author

btoews commented Jun 27, 2019

In d63b835, I decided to match Go's behavior by only returning the keyIdentifier field from the authorityKeyIdentifier extension. I think this is the part of the extension that most users will be looking for.

@btoews
Copy link
Contributor Author

btoews commented Jul 29, 2019

Sorry to be so slow to respond here. I got caught up with travel and work things. I think I've responded to your comments on the latest changes.

@btoews
Copy link
Contributor Author

btoews commented Sep 26, 2019

@ioquatix Is this good to merge?

@ioquatix ioquatix merged commit 1219f70 into ruby:master Sep 27, 2019
@ioquatix
Copy link
Member

Looks good to me. Merged.

@btoews btoews deleted the x509-extension-helpers branch September 27, 2019 15:06
@btoews
Copy link
Contributor Author

btoews commented Sep 27, 2019

Thanks

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

Successfully merging this pull request may close these issues.

2 participants