envoyproxy / envoy

Cloud-native high-performance edge/middle/service proxy
https://www.envoyproxy.io
Apache License 2.0
24.71k stars 4.75k forks source link

Expose well-known certificate subject fields in Lua filter, such as CN and O. #35524

Open zhishi opened 1 month ago

zhishi commented 1 month ago

Title: Expose well-known certificate subject fields in Lua filter, such as CN and O.

Description:

Today, Lua code can access certificate subject in RFC 2253 format via connection():ssl():subjectPeerCertificate() (doc). However, the subject string is kind of useless for code, because usually what we need is checking some field value (such as CN) in the subject. And parsing RFC 2253 string is pretty tricky (just read RFC4514 and RFC2253 will get the idea). People often use a simple regex to extract the field value which can be easily attacked by malicious certificate with escaped characters and put the service into security risk.

So I think it's better to expose the fields value from subject in Lua API directly, since internally envoy already parsed the certificate from ASN.1 format into data structures, directly return those fields actually avoid both generating RFC2253 and parsing RFC2253 string.

And we probably don't need expose every fields (since some ASN.1 type just can not be converted to string), just some well-known ones are good enough for most user scenarios. The purpose is not creating a strict X509 Name object, but reducing user's extra logic and security risk to access these common subject fields. A good example is the golang's pkix Name implementation, which exposed these fields:

    "2.5.4.6":  "C",
    "2.5.4.10": "O",
    "2.5.4.11": "OU",
    "2.5.4.3":  "CN",
    "2.5.4.5":  "SERIALNUMBER",
    "2.5.4.7":  "L",
    "2.5.4.8":  "ST",
    "2.5.4.9":  "STREET",
    "2.5.4.17": "POSTALCODE",

Relevant Links: RFC2253 RFC4514

soulxu commented 1 month ago

cc @wbpcode

zhishi commented 1 month ago

Since there is no response yet, can I contribute the PR to add this feature? Currently I'm working on a PoC PR, but I guess there are probably coding practice and c++ code structure rules in envoy I'm not fully aware of, that's why I think it's better to be done by envoy maintainer.

If that's ok here is the change: https://github.com/envoyproxy/envoy/compare/main...zhishi:envoy:zhimengshi/subject_rdn currently it only provide CN and O fields (other fields can be added easily). I want to make sure the interface and logic make sense before adding more code and unit tests.

At a glance it maybe simpler to directly add per field methods into ConnectionInfo, but this feature can be expanded to peer/local certificate and issue/subject fields, that will be too many methods in ConnectionInfo. So I introduced a new class to group those fields. Which is only for grouping those fields as a readonly structure, so I make it a simple struct without any methods. Hopes this is not breaking coding rules (like every class must based on a pure virtual class and only using methods etc.)

An example of the Lua code:

function envoy_on_request(handle)
  local subject = handle:connection():ssl():parsedSubjectPeerCertificate()
  if subject then
    local cn = subject:commonName()
    local o = table.concat(subject:organizationName(), ",")
  ...
end
github-actions[bot] commented 1 week ago

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

keithmattix commented 1 week ago

@ggreenway @wbpcode could one of you take a look here and see if the code changes @zhishi proposed look reasonable to you? I think this is a valid use-case

ggreenway commented 1 week ago

@zhishi can you post a PR adding one of the accessors you're planning? We can evaluate the API better in the context of the code needed to implement it.

zhishi commented 1 week ago

thanks for the response, I'll put PR soon. Although it works already, but I need to add unit tests to make it a ready PR. I hope this can provide enough context of the code change.