feat: make rustls cert public

[shaneutt]

This enables a deeper level of control from implementations utilizing Pingora over certificates.

This is being paired with #726 in our fork right now.

[johnhurt]

@fabian4 @nojima — could you review this when you get a chance?

[nojima]

@shaneutt
Could you please explain how you are using raw_cert and cert? Do both of these need to be public? Or would it be sufficient to expose only a few methods, such as borrow_cert()?

[shaneutt]

@shaneutt Could you please explain how you are using raw_cert and cert? Do both of these need to be public? Or would it be sufficient to expose only a few methods, such as borrow_cert()?

Thank you for your time @nojima!

The primary need is constructing WrappedX509 values from DER bytes outside the pingora-core crate. In Praxis, we cache CA certificate DER bytes at config time and convert them into WrappedX509 values for assignment to HttpPeer.options.ca when configuring per-upstream TLS verification:

upstream_peer.rs L105-L111:

fn ca_from_cached(                                                                                                                                           
    cached: &praxis_tls::CachedCaCerts,                   
) -> Vec<pingora_core::utils::tls::WrappedX509> {                                                                                                            
    cached                                                                                                                                                   
        .der_certs()                                                                                                                                         
        .iter()                                                                                                                                              
        .map(|der| {                                      
            pingora_core::utils::tls::WrappedX509::new(                                                                                                      
                der.clone(),                                                                                                                                 
                pingora_core::utils::tls::parse_x509,
            )                                                                                                                                                
        })                                                
        .collect()
}

Called during upstream peer selection to populate per-cluster CA roots.

So what this PR provides, broken down:

1. pub_extras makes WrappedX509::new() public (ouroboros gates the constructor on this, not on field visibility)
2. pub fn parse_x509() is needed as the builder argument to new()
3. pub fields make the ouroboros-generated borrow_cert() and borrow_raw_cert() accessors public (ouroboros ties accessor visibility to field visibility)

Items 1 and 2 are strictly required for our use case. Item 3 is not something we use today, but it seems generally useful for any downstream that needs to inspect certificate contents after construction. Let me know your thoughts? And again, thank you for your time 🙇

[nojima]

@shaneutt
Thank you for the use cases🙏

The approach makes sense, but I have a slight concern about the public API surface. I consider the fact that WrappedX509 is implemented using ouroboros to be an implementation detail, and I'd prefer not to leak it into the public interface. Similarly, parse_x509 doesn't seem like a good candidate for the crate's public API. So, instead of exposing these directly, how about adding a new function to WrappedX509?

impl WrappedX509 {
    pub fn parse(raw_cert: Vec<u8>) -> Self {
        Self::new(raw_cert, parse_x509)
    }
    ...
}

With this function, you can create WrappedX509 instances without exposing new or parse_x509, which should cover your use cases 1 and 2.

As for use case 3, we could support it by adding a new method rather than making the fields public:

    pub fn parsed(&self) -> &X509Certificate<'_> {
        self.borrow_cert()
    }

This approach avoids exposing the various ouroboros-generated methods, leaving us with more flexibility for future design decisions.

That said, I'm hesitant about implementing use case 3 right now. Removing a public function once it's been published is generally difficult. If you don't have an immediate need for it, I think it would be fine to address only use cases 1 and 2 in this PR and leave 3 for later.

[shaneutt]

This approach avoids exposing the various ouroboros-generated methods, leaving us with more flexibility for future design decisions.

That said, I'm hesitant about implementing use case 3 right now. Removing a public function once it's been published is generally difficult. If you don't have an immediate need for it, I think it would be fine to address only use cases 1 and 2 in this PR and leave 3 for later.

Sounds good 👍

Switched to that approach, and pushed it up. LMKWYT.

Note: I also included: dependency updates because audit was failing on reqwest; pinned actions to commit SHAs due to security restrictions for our fork. Let me know if you want me to drop anything.

[nojima]

@shaneutt

Note: I also included: dependency updates because audit was failing on reqwest; pinned actions to commit SHAs due to security restrictions for our fork. Let me know if you want me to drop anything.

A PR should contain changes for only a single purpose. If there are multiple independent changes, it's better to submit them as separate PRs rather than bundling them into one, so that each can be reviewed independently. Keeping PRs separate also simplifies things if a revert becomes necessary in the future.

In this case, I think this PR should be limited to just the feat: add parse on WrappedX509 impls commit, with everything else split out into a separate PR.

[shaneutt]

Yep, sounds fine. At least for now it passes CI, but I'm happy to strip it down 👍

[shaneutt]

@nojima things are stripped down, tests are added. And if you want, I added a small fix for the 1.91.1 build failures on main. Let me know what else you need here? 🖖

[nojima]

@shaneutt
Commit e5ea0df resolves the CI test failure, but the fix commit (e6e677f) has already been merged into the main branch. Therefore, please remove e5ea0df from this topic branch.

Also, please rebase this topic branch onto the main branch to pass the tests and resolve conflicts.

[shaneutt]

Updated! 🖖