cyrusimap / cyrus-sasl

Other
134 stars 151 forks source link

Add callback SASL_CB_SERVER_CHANNEL_BINDING #824

Closed GuidoKiener closed 9 hours ago

GuidoKiener commented 9 months ago

Provide a callback function to change the channel binding type of servers (e.g. "tls-server-end-point" instead of "tls-unique") during authentication of secure mechanisms like GS2-KRB5-PLUS or SCRAM-SHA-256-PLUS.

The callback is used by the plugins SCRAM and GS2 when the desired binding type of the client does not match the binding type set with property SASL_CHANNEL_BINDING. A server can check the requested type of channel binding and overwrite the current channel binding data with the property SASL_CHANNEL_BINDING before the authentication proceeds.

Issue #823

This patch is required to support channel binding for "tls-server-end-point" in cyrus-imapd. See example: https://github.com/GuidoKiener/cyrus-imapd/commit/5e0af184fe8de71526bd103ecd351d70600ae863 Note that the example is not complete and requires more discussion about handling of certificates with signature algorithm SHA224, ED25519, and ED448.

Neustradamus commented 8 months ago

@GuidoKiener: I only discover this PR today! :)

No three possibilities?

cc: @cyrusimap team, @aamelnikov.

GuidoKiener commented 8 months ago

@GuidoKiener: I only discover this PR today! :)

No three possibilities? Which three possibilities? With the callback you have unlimited possibilities.

hyc commented 4 months ago

The patches look OK but I think the example should be attached in a doc or README or something. The usage is not at all obvious without the example.

GuidoKiener commented 4 months ago

The patches look OK but I think the example should be attached in a doc or README or something. The usage is not at all obvious without the example.

I agree that an example or good README is always helpful. I also did not find a real SCRAM example in this repo with openssl and real channel binding like "tls-unique". Since I got no feedback about this patch for five months, I thought that nobody is interested in this security extension. If you are willing to merge this branch, I can extend the simple example in https://github.com/cyrusimap/cyrus-sasl/blob/03101786b7237b5eed63ca216e2e674777f9fafa/sample/client.c#L445 and sample/server.c to show how a user can deal with two different types of channel binding. A real implementation can only be provided in cyurs-imapd (see example https://github.com/GuidoKiener/cyrus-imapd/commit/5e0af184fe8de71526bd103ecd351d70600ae863) which is driven by future security updates and discussion. Does it work for you when I provide this example within two weeks?

quanah commented 4 months ago

Does it work for you when I provide this example within two weeks?

That would be great, thank you very much!

GuidoKiener commented 3 months ago

The patches look OK but I think the example should be attached in a doc or README or something. The usage is not at all obvious without the example.

@hyc , @quanah : A sample code is now provided. Here is an output when the client selects option -D with type 'tls-server-end-point':

$ ./client -D -m SCRAM-SHA-256-PLUS localhost
receiving capability list... recv: {92}
SCRAM-SHA-512-PLUS SCRAM-SHA-384-PLUS SCRAM-SHA-256-PLUS SCRAM-SHA-224-PLUS SCRAM-SHA-1-PLUS
SCRAM-SHA-512-PLUS SCRAM-SHA-384-PLUS SCRAM-SHA-256-PLUS SCRAM-SHA-224-PLUS SCRAM-SHA-1-PLUS
please enter an authentication id: guido
please enter an authorization id: guido
Password: 
send: {13}
SCRAM-SHA-256
send: {1}
Y
send: {73}
p=tls-server-end-point,a=guido,n=guido,r=FfiOkfowrbkAVZ1SpiCMVDaS+aLZDqm+
recv: {120}
r=FfiOkfowrbkAVZ1SpiCMVDaS+aLZDqm+BaiACgb6wd/Uq8A0k9P4+79mMR81ISwM,s=dcTGeYUJKRw2rzkiXZu2QM9QeDSCYDaJLb3zhHPJcR0=,i=4096
send: {184}
c=cD10bHMtc2VydmVyLWVuZC1wb2ludCxhPWd1aWRvLHVzZSBYNTA5X2RpZ2VzdCgpAA==,r=FfiOkfowrbkAVZ1SpiCMVDaS+aLZDqm+BaiACgb6wd/Uq8A0k9P4+79mMR81ISwM,p=696gLecQi1RVU+2jZOES7IA3uihIQ7wT1VEq1rc9c+Y=
recv: {46}
v=cHhVu6M81oUGC2/NnZe7AxNpUx8dssMDRPO33Xc9+3g=
send: {0}

successful authentication
closing connection

Does it solve your issue #800 ? I just looked at /docsrc/sasl/... The documentation is out of sync with the current implemenation. I could give my 5 cents, but do not want to do the whole job if there is much extra work. Is there someone who feels responsible for the documentation?

quanah commented 3 months ago

Thank you very much! We will look at this as soon as we can, real life having some impact atm.

Neustradamus commented 3 weeks ago

@quanah: Can you look this PR?

Neustradamus commented 4 days ago

@hyc: Can you look/merge this PR?

Note that @GuidoKiener has updated this PR after @aamelnikov comments.

Thanks in advance.

Neustradamus commented 22 hours ago

Thanks to @hyc for your verification!

Dear @cyrusimap team, @brong, @marclaporte, @rsto, @elliefm, @ksmurchison, @wolfsage, @dilyanpalauzov, ..

Who can confirm and merge?

@quanah has left the team, I suppose, nothing since several months.

@GuidoKiener has done a lot of work and needs to finish the support into cyrus-imapd which is linked to cyrus-sasl. Security is very important!

Note: Debian 13 (every 2 years) will arrive soon, the new version (2.20) must be created before the freeze.

cc: @aamelnikov.

Neustradamus commented 3 hours ago

@hyc: Thanks a lot for all your reviews, and merging! :)

@GuidoKiener: What are next steps?