a2 icon indicating copy to clipboard operation
a2 copied to clipboard

Token-based authentication without openssl

Open boxdot opened this issue 2 years ago • 15 comments

Add two feature flags: openssl (default) and ring (optional). The ring feature flags implements token-based authentication without dependency on openssl. However, it does not support PKCS#12-certificated authentication.

When both features are enabled: openssl is chosen over ring. At least one of the two features has to be enabled.

MSRV 1.60 (due to the feature section in Cargo.toml using dep:)

boxdot avatar Jun 10 '22 17:06 boxdot

👋🏼 Hello, this project now has new maintainers and over the coming week or so I'm going to do a full dive into each open PR/Issue. Is there the use-case of needing token based auth without openssl?

HarryET avatar Aug 15 '22 09:08 HarryET

Is there the use-case of needing token based auth without openssl?

openssl is a quite heavy dependency. I understand that not everybody tries to avoid it, but for some project it is too heavy. In my case, it is replaced by rustls (for TLS) and ring (for cryptography primitives).

boxdot avatar Aug 15 '22 09:08 boxdot

Is there the use-case of needing token based auth without openssl?

openssl is a quite heavy dependency. I understand that not everybody tries to avoid it, but for some project it is too heavy. In my case, it is replaced by rustls (for TLS) and ring (for cryptography primitives).

I see now, yeah could be a good thing to merge in. Any chance you can change this to target the v0.7 branch? and I'll get a review to you asap.

HarryET avatar Aug 15 '22 11:08 HarryET

Hey @boxdot, please review the comments left by myself and @ennmichael. Also please fix the Clippy errors.

HarryET avatar Aug 24 '22 09:08 HarryET

Thanks for your PR! Hopefully some of this feedback is constructive.

Thank you. It is very helpful.

boxdot avatar Aug 24 '22 09:08 boxdot

Please note, to enable ring, openssl has to be disable now. And at least one of the two features has to be enabled (--no-default-features does not compile due to explicit compilation error).

boxdot avatar Aug 24 '22 09:08 boxdot

Please note, to enable ring, openssl has to be disable now. And at least one of the two features has to be enabled (--no-default-features does not compile due to explicit compilation error).

Great!

sistemd avatar Aug 24 '22 10:08 sistemd

Please note, to enable ring, openssl has to be disable now. And at least one of the two features has to be enabled (--no-default-features does not compile due to explicit compilation error).

Just to clarify if both are enabled it will still work, correct?

HarryET avatar Aug 24 '22 11:08 HarryET

Just to clarify if both are enabled it will still work, correct?

Yes, if both are enabled, openssl takes precedence and is used.

boxdot avatar Aug 24 '22 12:08 boxdot

Just going to wait for a final review from @ennmichael and maybe another person from WalletConnect before merge

HarryET avatar Aug 28 '22 07:08 HarryET

Just to keep you updated, I'm away from work for a week or so but will try to get a review from someone else for you

HarryET avatar Aug 30 '22 20:08 HarryET

LGTM - only small request - can you update the Features section in the README to reflect this before pushing pls?

Updated

boxdot avatar Sep 05 '22 08:09 boxdot

Will review this on Monday and merge if all looks good, thanks again for the PR and sorry for the delay.

HarryET avatar Sep 10 '22 15:09 HarryET

Maybe one thing that could be added here is to get GitHub actions running without OpenSSL and using ring, at least clippy/format/compilation to see nothing breaks in the future.

pimeys avatar Sep 11 '22 08:09 pimeys

Maybe one thing that could be added here is to get GitHub actions running without OpenSSL and using ring, at least clippy/format/compilation to see nothing breaks in the future.

I added a jobs to run clippy and tests with the ring feature enabled only

boxdot avatar Sep 15 '22 10:09 boxdot

Thank you for reviewing.

JFYI: I don't have permissions to merge it btw.

boxdot avatar Sep 22 '22 15:09 boxdot

Thank you for reviewing.

JFYI: I don't have permissions to merge it btw.

Yeah, just a bit of a delay in merging from my end. Going to do it now; thanks again for the contributions.

HarryET avatar Sep 22 '22 17:09 HarryET