Skip to content

Add SCRAM-SHA-1 and SCRAM-SHA-256 support via libgsasl #6372

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Add SCRAM-SHA-1 and SCRAM-SHA-256 support via libgsasl #6372

wants to merge 1 commit into from
+261 −0

Conversation

jas4711
Copy link
Contributor

@jas4711 jas4711 commented Dec 25, 2020 •
edited
Loading

Hi! SCRAM offers some advantages compared to CRAM-MD5/DIGEST-MD5, and this is a proof-of-concept patch to add support for it via libgsasl. I am looking for help and review with the patch.

@jas4711 jas4711 changed the title SCRAM-SHA-1 and SCRAM-SHA-256 support via libgsasl Add SCRAM-SHA-1 and SCRAM-SHA-256 support via libgsasl Dec 25, 2020
bagder
bagder reviewed Dec 26, 2020
View reviewed changes
@Neustradamus
Copy link

@jas4711: Thanks for your SCRAM improvements.

Like you know, it is linked to: "RFC6331: Moving DIGEST-MD5 to Historic" since July 2011:

Please look @bagder comments.

@bagder: Thanks for your quick comments about this @jas4711 PR :)
It is an important PR.

@jas4711
sasl: Support SCRAM-SHA-1 and SCRAM-SHA-256 via libgsasl.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
jas4711 Simon Josefsson
GPG key ID: 51722B08FE4745A2
Expired
Verified
Learn about vigilant mode
517841f
@jas4711 jas4711 force-pushed the jas4711/gsasl-scram branch from 86a9c52 to 517841f Compare January 19, 2021 18:08
@Neustradamus
Copy link

@bagder: Have you a date of merging?

@bagder
Copy link
Member

bagder commented Feb 7, 2021

Within days I'd say.

@bagder
Copy link
Member

bagder commented Feb 9, 2021

Thanks!

@bagder bagder closed this in 3eebbfe Feb 9, 2021
@Neustradamus
Copy link

@jas4711: Thanks for your SCRAM improvements!

Thanks @bagder for the merging! :)

@vszakats
Copy link
Member

vszakats commented Feb 9, 2021 •
edited
Loading

Getting these errors and warnings when building against latest stable libgsasl 1.10.0 [UPDATE: there is a newer, 1.11.0, alpha versions, that answers some questions below.]:

clang -I. -I../include -I"../../nghttp2/pkg/usr/local/include" -I"../../libssh2/include" -I"../../libssh2/win32" -I"../../openssl/include" -I"../../zlib/pkg/usr/local" -I"../../zstd/build/cmake/pkg/usr/local/include" -I"../../libgsasl/pkg/usr/local/include" -target x86_64-w64-mingw32 --sysroot /usr/local/opt/mingw-w64/toolchain-x86_64 -DCURL_STATICLIB -DCURL_ENABLE_MQTT -fno-ident -DHAVE_ATOMIC -DUSE_HSTS -DUNICODE -D_UNICODE -DCURL_DISABLE_OPENSSL_AUTO_LOAD_CONFIG -DNGHTTP2_STATICLIB -g -O2 -Wall -W -fno-strict-aliasing -m64 -D_AMD64_ -DBUILDING_LIBCURL -DCURL_WITH_MULTI_SSL -DUSE_NGHTTP2 -DUSE_LIBSSH2 -DHAVE_LIBSSH2_H -DUSE_OPENSSL -DHAVE_OPENSSL_PKCS12_H -DOPENSSL_NO_KRB5 -DHAVE_OPENSSL_SRP -DUSE_TLS_SRP -DUSE_SCHANNEL -DHAVE_LIBZ -DHAVE_ZLIB_H -DHAVE_ZSTD -DUSE_WIN32_IDN -DWANT_IDN_PROTOTYPES -DUSE_GSASL -DUSE_WINDOWS_SSPI -DENABLE_IPV6 -D_WIN32_WINNT=0x0501 -DHAVE_LDAP_SSL -c vauth/krb5_gssapi.c -o vauth/krb5_gssapi.o
vauth/gsasl.c:70:7: error: assigning to 'int' from incompatible type 'void'
  res =
      ^
vauth/gsasl.c:75:63: error: use of undeclared identifier 'result'
    failf(data, "setting AUTHID failed: %s\n", gsasl_strerror(result));
                                                              ^
vauth/gsasl.c:81:7: error: assigning to 'int' from incompatible type 'void'
  res =
      ^
vauth/gsasl.c:86:65: error: use of undeclared identifier 'result'
    failf(data, "setting PASSWORD failed: %s\n", gsasl_strerror(result));
                                                                ^
vauth/gsasl.c:111:38: warning: passing 'unsigned char *' to parameter of type 'const char *' converts between pointers to integer types with different sign [-Wpointer-sign]
  result = gsasl_step(gsasl->client, chlg, chlglen, &response, outlen);
                                     ^~~~
../../libgsasl/pkg/usr/local/include/gsasl.h:445:20: note: passing argument to parameter 'input' here
                                   const char *input, size_t input_len,
                                               ^
vauth/gsasl.c:101:10: warning: unused variable 'chlg64len' [-Wunused-variable]
  size_t chlg64len = chlg64 ? strlen(chlg64) : 0;
         ^
2 warnings and 4 errors generated.
make[1]: *** [vauth/gsasl.o] Error 1

One is trivial to fix by renaming result references to res, but the rest raises questions, e.g. gsasl_property_set() returns void with GSASL_VERSION_NUMBER == 0x010a00.
[UPDATE: The correct condition is GSASL_VERSION_NUMBER >= 0x010a01. Would be nice to apply it to the NEWS entry of libgsasl as well. So the remaining question is the unused variable chlg64len.]

PR fixing the simpler ones: #6587

@vszakats vszakats mentioned this pull request Feb 9, 2021
Closed
vszakats added a commit to vszakats/curl that referenced this pull request Feb 10, 2021
@vszakats
gsasl: fix errors/warnings building against libgsasl

Verified

This commit was signed with the committer’s verified signature.
vszakats Viktor Szakats
GPG key ID: B5ABD165E2AEF201
Verified
Learn about vigilant mode
11ba377 
Ref: curl#6372 (comment)
@jas4711
Copy link
Contributor Author

jas4711 commented Feb 10, 2021

The API change is in the 1.11.x branch, and not in 1.10.x, so I think the check should be GSASL_VERSION_NUMBER >= 0x010b00 -- sorry about that!

I think the other issues are resolved? I can't really tell what is open and what is resolved. Thanks for merging, btw!

vszakats added a commit that referenced this pull request Feb 10, 2021
@vszakats
gsasl: fix errors/warnings building against libgsasl

Verified

This commit was signed with the committer’s verified signature.
vszakats Viktor Szakats
GPG key ID: B5ABD165E2AEF201
Verified
Learn about vigilant mode
62c4f2f 
- also fix an indentation
- make Curl_auth_gsasl_token() use CURLcode (by Daniel Stenberg)

Ref: #6372 (comment)
Ref: #6588

Reviewed-by: Jay Satiro
Assisted-by: Daniel Stenberg
Reviewed-by: Simon Josefsson
Closes #6587
@Crghilardi Crghilardi mentioned this pull request Sep 1, 2021
Merged
@Neustradamus Neustradamus mentioned this pull request Aug 1, 2022
Closed
@Neustradamus Neustradamus mentioned this pull request Feb 15, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bagder bagder

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jas4711 @Neustradamus @bagder @vszakats