Skip to content
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.

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

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

Closed
wants to merge 1 commit into from

Conversation

@jas4711
Copy link
Contributor

@jas4711 jas4711 commented Dec 25, 2020

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
lib/vauth/gsasl.c Outdated Show resolved Hide resolved
lib/vauth/gsasl.c Outdated Show resolved Hide resolved
lib/vauth/gsasl.c Outdated Show resolved Hide resolved
lib/vauth/gsasl.c Outdated Show resolved Hide resolved
@Neustradamus
Copy link

@Neustradamus Neustradamus commented Dec 26, 2020

@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 jas4711 force-pushed the jas4711/gsasl-scram branch from 86a9c52 to 517841f Jan 19, 2021
@Neustradamus
Copy link

@Neustradamus Neustradamus commented Feb 7, 2021

@bagder: Have you a date of merging?

@bagder
Copy link
Member

@bagder bagder commented Feb 7, 2021

Within days I'd say.

@bagder
Copy link
Member

@bagder bagder commented Feb 9, 2021

Thanks!

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

@Neustradamus Neustradamus commented Feb 9, 2021

@jas4711: Thanks for your SCRAM improvements!

Thanks @bagder for the merging! :)

@vszakats
Copy link
Member

@vszakats vszakats commented Feb 9, 2021

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 added a commit to vszakats/curl that referenced this issue Feb 9, 2021
vszakats added a commit to vszakats/curl that referenced this issue Feb 9, 2021
Known issue: Unused `chlg64len` still needs to be fixed.

Ref: curl#6372 (comment)
vszakats added a commit to vszakats/curl that referenced this issue Feb 10, 2021
@jas4711
Copy link
Contributor Author

@jas4711 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 issue Feb 10, 2021
- 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants