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

Switch base64 decoding to data-encoding crate #136

Merged
merged 1 commit into from
Mar 8, 2023

Conversation

sdroege
Copy link
Contributor

@sdroege sdroege commented Jan 16, 2023

It was already previously used elsewhere in this crate and depending on two base64 implementations doesn't seem ideal.


Also fixes #134

@chifflier
Copy link
Member

Oh, I missed this when merging #138

This looks interesting, but we'd need to ensure that the features provided are really similar. Do you think this is the case?
If yes, could you rebase and resubmit? Thanks!

@sdroege
Copy link
Contributor Author

sdroege commented Mar 7, 2023

It's the same base64 variant, if that's your question :)

https://docs.rs/data-encoding/latest/data_encoding/ has a comparison between data-encoding, base64 and GNU base64 for some other properties.

If this sounds good to you I'll rebase the PR.

@chifflier
Copy link
Member

Yes, in the past I encountered some weird base 64 variants (some remove the trailing ==, others are said web-safe, etc) and I did not check the RFC to find which is used in certificates. It does not seem to be an exotic one.
I'm fine switching crates, so please go ahead

It was already previously used elsewhere in this crate and depending on
two base64 implementations doesn't seem ideal.
@sdroege
Copy link
Contributor Author

sdroege commented Mar 8, 2023

Ok, I've rebased this now. Let me know if this look ok to you :)

@chifflier chifflier merged commit 0b7ff50 into rusticata:master Mar 8, 2023
@chifflier
Copy link
Member

Merged, thanks!

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.

None yet

2 participants