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

Jump to bottom

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

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

Conversation

jas4711
Copy link
Contributor

@jas4711 jas4711 commented Dec 25, 2020 •
edited

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

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

@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

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 pull request Feb 9, 2021
@vszakats
gsasl: fix errors/warnings building against libgsasl
7eb1484 
Ref: curl#6372 (comment)
vszakats added a commit to vszakats/curl that referenced this pull request Feb 9, 2021
@vszakats
gsasl: fix errors/warnings building against libgsasl
3df53c1 
Known issue: Unused `chlg64len` still needs to be fixed.

Ref: curl#6372 (comment)
@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
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
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 bagder left review comments

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