This one is especially important for a mechanism like EXTERNAL. Callbacks are per-config and have no per-session data making it very annoying to make decisions based on e.g. a provided TLS client certificate.

A possible solution would be to also use the property system here extending the SessionData passed into callbacks already.

I'm also very much open to alternative suggestions!

This is a good first issue since SessionData is only seen by callback and (possibly) the protocol crate and the property system could be used as-is if that ends up being the chose solution.

Hello there 👋

I had an implementation issue with the 'static constraint for my callback :

development...Mathieu-Lala:rsasl:fix/callback-static

What do you think about these changes ?
Is taking a Box<dyn SessionCallback> a problem ?

Before opening a new issue to request a mechanism to be supported:

Keep in mind that rsasl doesn't require mechanism support to live in this crate. It's very much possible — and encouraged — for other crates to add support for new mechanisms.

Still, for some mechanism it makes sense for rsasl itself to provide. Right now our guidelines for that are quite simple:

Mechanisms registered with IANA under "COMMON" Usage

These mechanisms we generally do want to provide, no questions asked. If the mechanism you want is one of those, go ahead and open an issue for it or, even better, a PR adding support.

Planned support

Support for the following mechanisms is already planned:

• GS2-* (See #25)

Requested

Support for the following mechanisms was requested and would be sensible to be included in rsasl directly, but aren't planned to be implemented by me at the moment (PRs welcome!)

• SCRAM-SHA512(-PLUS) (See #26)

Requested but not planned

Support for the following mechanisms was requested but is not planned to be added to rsasl itself. Keep in mind though that rsasl is explicitly designed so that other crates can provide mechanism implementations without having to change rsasl in any way, so all mechanisms listed here can still be made available in the rsasl framework even if not implemented in rsasl itself!

If you need support for these mechanism please do consider spearheading an implementation by going to their respective issues and coordinating such with other the other users.

• NTLM (See #21)

Help to add those or other mechanism is however always apprechiated!

It would be great if the examples directory could be expanded with an example of creating a GSSAPI (Kerberos) client.
Thanks in advance.

Hello!

I just started using your library and thought sharing my experience (as an rsasl newbie) could help improve it. I was confused about the name step64 because I expected it to have something to do with 32-bit vs 64-bit systems. Maybe it's just me.

But maybe step_base64 would be a better name?

PS: I really hope to get the rsasl integration done soon. As far as I see, this could work out really well 🙏🏻 So... thanks a lot for this crate!

Using the GSSAPI mechanism fails to build on macOS, it looks like the only way to build libgssapi on macs is to disable the default features: estokes/libgssapi#2

Jobs that are superseded by another CI run should be canceled. See duesee/imap-codec#92

TLDR;

We could add ...

concurrency:
  group: ${{ github.workflow }}-${{ github.ref }}
  cancel-in-progress: true

... to all workflow files to eagerly cancel jobs that won't matter in the future.

I could make a PR if you like this change :-)

As auto traits of public types are part of our public API we must take care not to break semver here (see also #15).

We already have the static_assertions crate as dev-dependency that can help test.

This is a good first issue as writing the tests only requires looking at the public API and as it's #[cfg(test)]-only code can not break downstream users.

As a reasonable approximation "more testing" = "more good" does hold. And rsasl already does some testing but it could definitely profit from more.

Especially as it's a rather abstract framework integration tests are relevant to make sure updates don't break stuff.

Especially integration tests that just test the client portion of a mechanism against its server portion should be a good place to start as they won't require knowledge of the inner workings of the mechanisms or rsasl itself.

Right now there is a potential issue with Callbacks that need to aquire locks; if a mechanism calls session.need_with and friends from within the closure of session.need_with a second call to Callback::callback will be issued before the first one completes. If e.g. a Mutex is aquired inside the first function call it will likely not be released by the second call, leading to a deadlock.
We should either explicitly document Callbacks not being allowed to hold locks over a call to provide or prevent mechanism from calling need_with recursively.

The latter can easily be done via type system by making need_with take &mut self, the latter is likely much harder and potentially worse from an user perspective.

A few things have changed since the documentation for them was written.

Immediately to mind come things like the Registry and Properties and downstream implementation of those.

The documentation for those needs to be updated before we make a stable 2.0.0 release.

The idea comes from this twitter thread: https://twitter.com/jonhoo/status/1571290371124260865

The basic idea is to make sure dependencies marked in the Cargo.toml have correct lower bounds — in other words that with the minimum possible version of all dependencies code still compiles and still passes tests. To reduce friction with PRs a Cargo.lock should also be committed into the repository and tests be run with --locked.

This is a good first issue especially for developers that have or want experience with GitHub actions; Jon Gjengset is also maintainer of the imap crate so it can serve as inspiration: https://github.com/jonhoo/rust-imap

This is mostly important for enums or structs with public fields; exhaustive pattern matching breaks if they are extended without care and haven't been marked correctly (using e.g. #[non_exhaustive]).

I would very much apprechiate additional eyes on this one!

This is a good first issue as it only concerns the peripheral API and it's very much not required to know all internal details of rsasl to help with this.

Should step{,64} be allowed to panic, e.g., when called after Finished or due to other errors?

LOGIN

Stepping through LOGIN seems to "fuse" the stepping. This is what I would expect. But maybe there are reasons not to.

* ok [capability auth=login] asd
A1 AUTHENTICATE LOGIN
+ ...
QWzCuWNl
+ ...
UGHCssKydzByZA==
+ ...
*// I send this due to `error=MechanismDone`
+ ...
*// I send this due to `error=MechanismDone`
+ ...
*// I send this due to `error=MechanismDone`

PLAIN

Stepping through PLAIN seems to repeat the last step. Is this how it should be? :-)

* ok [capability auth=plain] asd
A1 AUTHENTICATE PLAIN
+ ...
AEFswrljZQBQYcKywrJ3MHJk
+ ...
AEFswrljZQBQYcKywrJ3MHJk
+ ...
AEFswrljZQBQYcKywrJ3MHJk
+ ...
AEFswrljZQBQYcKywrJ3MHJk
+ ...
AEFswrljZQBQYcKywrJ3MHJk

SCRAM-SHA-256

Stepping through SCRAM-SHA-256 (with bad input) panics when we call step after we receive the first error.

* ok [capability auth=scram-sha-256] asd
A1 AUTHENTICATE SCRAM-SHA-256
+ ...
biwsbj1BbMK5Y2Uscj0qTWNzYmN3XjFEMSl1NWxgJ3RgPiNmYWc=
+ ...
*// I send this due to `error=InputDataRequired`
+ ...
// thread 'main' panicked at rsasl-2.0.0/src/mechanisms/scram/client.rs:485:21:
State machine in invalid state

While working on #32, I thought for a moment that I messed something up (see screenshot below). But it seems that the SCRAM_*_PLUS statics are not feature-gated by registry_static. I guess they should be?

Hello there 👋

I have been updating rsasl to the latest version on our project : https://github.com/viridIT/vSMTP/pull/536
But my test with the PLAIN protocol fails now, where before they were passing (see CI result).

Can you tell me if you test are indeed wrong, or if there is a problem with the PLAIN implementation ?

Also, I saw that you do not have CI yet in your project, I can add these to your repo if you wish https://github.com/viridIT/vSMTP/blob/develop/.github/workflows/ci.yaml

And if you needs hands to write unit tests, do not hesitate to create an issue with the label help wanted

Thank you a lot for you works

The system around struct Tagged and trait Erased powers most of the very central Property-based callback system. It's also the source of most of our unsafe and the only one that needs to be very careful with lifetimes.

I'm 90% certain I have gotten it correct and that the code is safe, but I'd like to be 100% on this. Additionally I'd like future me to still be able to quickly read through the comments and understand the assertions the code has that makes it safe.

At this time, the last stable gsasl version supports:

• SCRAM-SHA-1
• SCRAM-SHA-1-PLUS
• SCRAM-SHA-256
• SCRAM-SHA-256-PLUS

Can you add more SCRAM supports?

• https://www.gnu.org/software/gsasl/

Yeah, that one. I actually need it too so … yeah.

Building rsasl on linux/aarch64 and linux/armv7 fails with the following error:

Compiling rsasl v1.4.0
 error[E0308]: mismatched types
    --> /usr/local/cargo/registry/src/github.com-1ecc6299db9ec823/rsasl-1.4.0/src/lib.rs:276:61
     |
 276 |         let ret = unsafe { gsasl_client_support_p(self.ctx, mech.to_bytes_with_nul().as_ptr() as *const i8) };
     |                                                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `u8`, found `i8`
     |
     = note: expected raw pointer `*const u8`
                found raw pointer `*const i8`
 
 error[E0308]: mismatched types
    --> /usr/local/cargo/registry/src/github.com-1ecc6299db9ec823/rsasl-1.4.0/src/lib.rs:287:61
     |
 287 |         let ret = unsafe { gsasl_server_support_p(self.ctx, mech.as_ptr() as *const i8) };
     |                                                             ^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `u8`, found `i8`
     |
     = note: expected raw pointer `*const u8`
                found raw pointer `*const i8`
 
 For more information about this error, try `rustc --explain E0308`.
 error: could not compile `rsasl` due to 2 previous errors

This seems to be caused by c_char being a u8 on those platforms..
Similar issue with PyO3for reference: PyO3/pyo3#1181

rsasl version up to 1.2.0 will derefence a NULL valued pointer when asked to translate an unknown error code into a human-readable string.
The issue was fixed in 72e12de included in release 1.2.0

Originally posted by @Mathieu-Lala in #19 (comment)

It doesn't need to be and (correctly) is complained at by https://rust-lang.github.io/rust-clippy/master/

Since ScramServer is a private struct, how do I add SCRAM-SHA512 and SCRAM-SHA512-PLUS support?

Or, do you have any plan to add those support?

In the code, I see you have tried to add sha512 support but why comment it later?

#[cfg(feature = "scram-sha-1")]
pub type ScramSha1Server<const N: usize> = ScramServer<sha1::Sha1, N>;
#[cfg(feature = "scram-sha-2")]
pub type ScramSha256Server<const N: usize> = ScramServer<sha2::Sha256, N>;
// #[cfg(feature = "scram-sha-2")]
// pub type ScramSha512Server<const N: usize> = ScramServer<sha2::Sha512, N>;

I'm using version 2.0.0-preview9. It seems there is currently no way to extract the authenticated identity after running the SCRAM mechanism successfully on the server side. Comparing with the code for PLAIN I would expect that SCRAM also calls SessionCallback::validate() when it's finished. Is this still missing or is there some other way to extract the identity after authentication?

Splitting up the callback into a client & server variant makes sense as most users of this crate are either or, not both. This would also allow a better typing of the Callbacks as e.g. client callbacks don't need to implement validate, while server callbacks have to.

This would also allow a protocol implementation to require Callbacks with a given specialization. A server callback could thus statically type the additional data provided by SessionData and the type required for validate:

pub trait Specialization {
    /// Additional data provided by the protocol implementation in callbacks.
    type SessionData;

    /// Validation type returned on successful authentication
    type Validation;

    /// Error type returned specifically on failed authentication, can indicate type of failure.
    type ValidationError;
}

impl<S: Specialization> SessionData<S> {
    [...]
    fn protocol_session_data(&self) -> &S::SessionData { ... }
    [...]
}

pub trait ServerCallback<Specialization> {
    fn callback(&self, 
                session_data: &SessionData<Specialization::SessionData>,
                context: &Context,
                request: &mut Request
    ) -> Result<(), SessionError>;

    fn validate(&self, 
                session_data: &SessionData<Specialization>,
                context: &Context
    ) -> Result<Specialization::Validation, ValidationError<Specialization::ValidationError>>;
}

At the time of writing there are 9 unsafe blocks in rsasl code, 8 of which are in rsasl proper, with a final one in the SCRAM parser. Most of them are inherently safe casts, but the safety of them is not explicitly documented but based on tribal knowledge (e.g. #[repr(transparent)] struct A(B) meaning A(B) has the exact same representation as B and thus making a cast from &B to &A a safe no-op).

Those should be documented better.

Oh and I need to look at the more dodgy ones, like Mechname::const_new.

Extracted from #9 into it's own issue

The GSS-API based mechanism would be very good to support.

We take in and pass out several different types; types which should have very clear trait bounds that allow us some future iteration without breaking semver too.

In general the last point implies that we should have more specific requirements for types that are passed to us and guarantee much less traits for types we pass out.

This issue is especially relevant for auto-traits (Send, Sync, Unpin, UnwindSafe, …) that can easily be accidentally removed and then constitute a hidden breaking semver change.

Right now the features required for mechanisms to be available is written as plain text. Rustdoc has a feature to have the fact that a certain module needs a certain feature documented in a standart format.

It would be quite nice to switch to that as soon as that stabilizes.

SCRAM can use several different digest algorithms for authentication. IANA-Registered are SHA1 and SHA256, but implementations using SHA512 and even Argon2id are out there.

Right now the only way to retrieve the digest algorithm that's in use is via checking the mechanism name in SessionData, e.g. like so:

if session_data.mechanism().as_ref().starts_with("SCRAM-SHA256") {

This is somewhat annoying to say the least.

It would probably be nice to give access to the digest name via some property.

This is a good first issue as it's a very small and contained change in src/mechanisms/scram/client.rs and src/mechanisms/scram/server.rs