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

feat(tls): Add ability to set der encoded certificate #1748

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tottoto
Copy link
Collaborator

@tottoto tottoto commented Jun 22, 2024

Adds ability to set DER encoded certificate.

@tottoto tottoto force-pushed the add-ability-to-set-der-encoded-certificate branch from 703335e to 3332a51 Compare June 22, 2024 06:49
@djc
Copy link
Contributor

djc commented Jun 23, 2024

What's the motivating use case here? I feel like this adds quite a bit of API surface/complexity for stuff that's ultimately of little use when this API is mostly only needed as input stuff for building a transport. Also given that we're already using CertificateDer as output in the public API since #1707 I think it might be better to allow CertificateDer as input directly for Identity and Certificate (as opposed to all the AsRef<[u8]> generics.

@tottoto
Copy link
Collaborator Author

tottoto commented Jun 24, 2024

The use case is the case of when already having DER encoded certificates but not using tls-roots. Regarding using AsRef<[u8]> it is for API symmetry and the case users do not get CertificateDer directly.

However looking at the results after implementation, I thought that this is not worth the maintenance costs increasing, considering the main use case is solved by using tls-roots feature. And I am now inclined that it might be reasonable to have the implementation of these interfaces take CertificateDer directly. In this case, the users will have responsible for converting formats when they have PEM encoded certificates, but I am not convinced that whether it is a reasonable responsibility that users should have.

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

Successfully merging this pull request may close these issues.

2 participants