reown-com / a2

An Asynchronous Apple Push Notification (apns2) Client for Rust
MIT License
152 stars 53 forks source link

Token-based authentication without openssl #62

Closed boxdot closed 2 years ago

boxdot commented 2 years ago

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:)

HarryET commented 2 years ago

👋🏼 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?

boxdot commented 2 years ago

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).

HarryET commented 2 years ago

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 commented 2 years ago

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

boxdot commented 2 years ago

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

Thank you. It is very helpful.

boxdot commented 2 years ago

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).

sistemd commented 2 years ago

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!

HarryET commented 2 years ago

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?

boxdot commented 2 years ago

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

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

HarryET commented 2 years ago

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

HarryET commented 2 years ago

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

boxdot commented 2 years ago

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

Updated

HarryET commented 2 years ago

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

pimeys commented 2 years ago

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.

boxdot commented 2 years ago

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 commented 2 years ago

Thank you for reviewing.

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

HarryET commented 2 years ago

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.