open-policy-agent/opa

Provide SAN URIs as plain strings in crypto.x509.parse_certificates output

Closed this issue · 8 comments

kenjenkins commented

What is the underlying problem you're trying to solve?

For X.509 certificates containing a Subject Alternative Name uniformResourceIdentifier, the output returned by crypto.x509.parse_certificates seems difficult to work with.

It appears to return URIs as JSON encoded from the url.URL type.

For example, the URI spiffe://example.com/workload/id is returned as:

{
    "ForceQuery": false,
    "Fragment": "", 
    "Host": "example.com",
    "OmitHost": false,
    "Opaque": "", 
    "Path": "/workload/id",
    "RawFragment": "", 
    "RawPath": "", 
    "RawQuery": "", 
    "Scheme": "spiffe",
    "User": null
}

(see https://play.openpolicyagent.org/p/5Be0L3WxXD)

It seems non-trivial to write policies against this output. If I want to match an input string against this value, I'm not sure how to convert them to the same format. (At least, I don't see any built-in functions for converting a string to a url.URL or a url.URL back to a string.)

Describe the ideal solution

I think it might be more convenient to return any SAN URIs directly as they appear in the certificate, without first converting to the url.URL type. If the format of the URIs field cannot be changed for backwards-compatibility reasons, perhaps we could add a RawURIs field with the plain strings?

Describe a "Good Enough" solution

If there were a built-in function to convert from a url.URL back to a plain string, that could help when writing policies involving SAN URIs. (Though there might still be some issues if there are URIs that do not round-trip cleanly through conversion to url.URL and back.)

Additional Context

charlieegan3 commented

Hey, thanks for filing this detailed issue @kenjenkins, I think it's a valid feature request.

While the information is also available in the Extensions, e.g. https://play.openpolicyagent.org/p/bWUazuP9gs. You run into the same issue we're dealing with in #6268 of handling the ASN.1 data.

I personally think that the best option is to add an additional RawURIs field to the parsed certificate data as you suggest. I'm reluctant to consider a new built-in here since I think such a function wouldn't have a lot of other uses beyond this specific case.

anderseknert commented

If you just need something to quickly convert it to the original string, you could use a little helper function, like:

uri_to_string(uri) := sprintf("%s://%s%s", [uri.Scheme, uri.Host, uri.Path])

str := uri_to_string(crypto.x509.parse_certificates(input.cert)[0].URIs[0])

# str == spiffe://example.com/workload/id

Obviously, you might want to add fragment/query parts as well if those are significant.

charlieegan3 commented

I guess that SPIFFE ID URIs won't contain params & fragments (since it's not in their spec), but other use cases for the URI SAN might...

Another direction we could go with this would be to add RawURIs and SPIFFEID if a compliant SPIFFE ID is found in the cert's SANs. I've wondered about adding some SPIFFE built-ins before, e.g. spiffe_id_in_trust_domain etc. But at the moment, I don't think we have anything that's SPIFFE specific in OPA, so that would be a first.

kenjenkins commented

@anderseknert, I was concerned that something like sprintf("%s://%s%s", [uri.Scheme, uri.Host, uri.Path]) would not recover the original URI in all cases. Even extending this to support query etc. seems complicated (compare with the logic inside URL.String()).

Even if we define a built-in function that invokes the Go method URL.String() directly, I'm still not sure whether a round-trip through url.Parse() and URL.String() will always return the original input.

Will this matter in practice? I'm not sure, but if feels prudent to err on the side of caution.

@charlieegan3, I think if the ASN.1 parsing were limited to just the uniformResourceIdentifier elements inside the subjectAltName extension, maybe that would avoid some of the problems with otherName parsing?

I think we could extract just the URIs (tag 6) with something like this?

func(bctx rego.BuiltinContext, op1 *ast.Term) (*ast.Term, error) {
       var s cryptobyte.String
       if err := json.Unmarshal([]byte(op1.String()), &s); err != nil {
               return nil, err
       }

       var seq cryptobyte.String
       if !s.ReadASN1(&seq, asn1.SEQUENCE) {
               return nil, errors.New("error reading sequence")
       }

       var uriTag = asn1.Tag(6).ContextSpecific()

       var uris []*ast.Term
       for !seq.Empty() {
               var value cryptobyte.String
               var tag asn1.Tag
               if !seq.ReadAnyASN1(&value, &tag) {
                       return nil, errors.New("error reading name")
               } else if tag == uriTag {
                       uris = append(uris, ast.StringTerm(string(value)))
               }
       }
       return ast.ArrayTerm(uris...), nil
}
anderseknert commented

Sure, @kenjenkins! I was thinking only of the SPIFFE case, where as @charlieegan3 said, query params and fragments are out of spec, and something simple like that might be enough. Not as a generic solution.

charlieegan3 commented

Hey, thanks for the example @kenjenkins. I think that while it's related to #6268, we can do something simpler without needing to add a new built-in.

The structure is quite Go specific, but also, I think, only present in the output of this built-in.

{
    "ForceQuery": false,
    "Fragment": "", 
    "Host": "example.com",
    ...

I feel like adding a RawURIs field to the marshalled cert output is the best option. The implementation can also be quite simple: #6424 - let me know what you think.

kenjenkins commented

Thanks for implementing this, much appreciated!

charlieegan3 commented

You're welcome, thanks for the feature request. Keep us posted on how you're using OPA with SPIFFE, it's a topic close to my heart and I'm keen to make sure we have a good story for SPIFFE users in OPA. 🚀