Adding support for --ciphers in WinSSL/Schannel... #2630

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
2 participants
@RobertPragSymc
Contributor

RobertPragSymc commented Jun 2, 2018

Adding support for selecting ciphers in WinSSL/Schannel. Given the
contstraints of SChannel, I'm exposing these as the algorithms
themselves instead; while replicating the ciphersuite as specified by
OpenSSL would have been preferable, I found no way in the SChannel API
to do so.

To use this from the commandline, you need to pass the names of contants
defining the desired algorithms. For example:

curl --ciphers "CALG_SHA_256:CALG_RSA_SIGN:CALG_RSA_KEYX:CALG_AES_128:CALG_DH_EPHEM" https://github.com

The specific names come from wincrypt.h

This is an attempt to implement
https://curl.haxx.se/docs/todo.html#Add_support_for_the_ciphers_op
limited by the oddities of how SChannel handles encryption algorithms.

Adding support for selecting ciphers in WinSSL/Schannel. Given the co…
…ntstraints of SChannel, I'm exposing these as the algorithms themselves instead; while replicating the ciphersuite as specified by OpenSSL would have been preferable, I found no way in the SChannel API to do so.

To use this from the commandline, you need to pass the names of contants defining the desired algorithms. For example,
curl --ciphers "CALG_SHA1:CALG_RSA_SIGN:CALG_RSA_KEYX:CALG_AES_128:CALG_DH_EPHEM" https://github.com
The specific names come from wincrypt.h
@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Jun 2, 2018

Member

Remember make checksrc to validate the code style:

/vtls/schannel.c:211:3: warning: if with space (SPACEBEFOREPAREN)
 if (strcmp(#X, tmp) == 0) \
   ^
./vtls/schannel.c:215:25: warning: no space after declarative asterisk (ASTERISKNOSPACE)
 get_alg_id_by_name(char* name)
                         ^
./vtls/schannel.c:215:24: warning: no space before asterisk (ASTERISKNOSPACE)
 get_alg_id_by_name(char* name)
                        ^
./vtls/schannel.c:219:104: warning: Longer than 79 columns (LONGLINE)
   size_t n = nameEnd ? min(nameEnd - name, LONGEST_ALG_ID - 1) : min(strlen(name), LONGEST_ALG_ID - 1);
./vtls/schannel.c:297:52: warning: no space after declarative asterisk (ASTERISKNOSPACE)
 set_ssl_ciphers(SCHANNEL_CRED *schannel_cred, char* ciphers)
                                                    ^
./vtls/schannel.c:297:51: warning: no space before asterisk (ASTERISKNOSPACE)
 set_ssl_ciphers(SCHANNEL_CRED *schannel_cred, char* ciphers)
                                                   ^
./vtls/schannel.c:304:7: warning: if with space (SPACEBEFOREPAREN)
     if (!alg)
       ^
./vtls/schannel.c:306:7: warning: if with space (SPACEBEFOREPAREN)
     if (alg)
       ^
./vtls/schannel.c:311:7: warning: if with space (SPACEBEFOREPAREN)
     if (startCur)
       ^
./vtls/schannel.c:535:4: warning: Trailing whitespace (TRAILINGSPACE)
     
    ^
./vtls/schannel.c:539:82: warning: Longer than 79 columns (LONGLINE)
         failf(data, "Unable to set ciphers to %s", SSL_CONN_CONFIG(cipher_list));
Member

bagder commented Jun 2, 2018

Remember make checksrc to validate the code style:

/vtls/schannel.c:211:3: warning: if with space (SPACEBEFOREPAREN)
 if (strcmp(#X, tmp) == 0) \
   ^
./vtls/schannel.c:215:25: warning: no space after declarative asterisk (ASTERISKNOSPACE)
 get_alg_id_by_name(char* name)
                         ^
./vtls/schannel.c:215:24: warning: no space before asterisk (ASTERISKNOSPACE)
 get_alg_id_by_name(char* name)
                        ^
./vtls/schannel.c:219:104: warning: Longer than 79 columns (LONGLINE)
   size_t n = nameEnd ? min(nameEnd - name, LONGEST_ALG_ID - 1) : min(strlen(name), LONGEST_ALG_ID - 1);
./vtls/schannel.c:297:52: warning: no space after declarative asterisk (ASTERISKNOSPACE)
 set_ssl_ciphers(SCHANNEL_CRED *schannel_cred, char* ciphers)
                                                    ^
./vtls/schannel.c:297:51: warning: no space before asterisk (ASTERISKNOSPACE)
 set_ssl_ciphers(SCHANNEL_CRED *schannel_cred, char* ciphers)
                                                   ^
./vtls/schannel.c:304:7: warning: if with space (SPACEBEFOREPAREN)
     if (!alg)
       ^
./vtls/schannel.c:306:7: warning: if with space (SPACEBEFOREPAREN)
     if (alg)
       ^
./vtls/schannel.c:311:7: warning: if with space (SPACEBEFOREPAREN)
     if (startCur)
       ^
./vtls/schannel.c:535:4: warning: Trailing whitespace (TRAILINGSPACE)
     
    ^
./vtls/schannel.c:539:82: warning: Longer than 79 columns (LONGLINE)
         failf(data, "Unable to set ciphers to %s", SSL_CONN_CONFIG(cipher_list));
@RobertPragSymc

This comment has been minimized.

Show comment
Hide comment
@RobertPragSymc

RobertPragSymc Jun 6, 2018

Contributor

Let me know if there is anything further I should do around this change.

Contributor

RobertPragSymc commented Jun 6, 2018

Let me know if there is anything further I should do around this change.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Jun 7, 2018

Member

I think it'd be good to also get the ciphers documented in docs/CIPHERS.md !

Member

bagder commented Jun 7, 2018

I think it'd be good to also get the ciphers documented in docs/CIPHERS.md !

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Jun 11, 2018

Member

@RobertPragSymc if you can just amend the docs, I'm ready to merge!

Member

bagder commented Jun 11, 2018

@RobertPragSymc if you can just amend the docs, I'm ready to merge!

Adding the constants used to CIPHERS.md, and documenting the fact that
encryption algorithm selection is available in WinSSL.
@RobertPragSymc

This comment has been minimized.

Show comment
Hide comment
@RobertPragSymc

RobertPragSymc Jun 12, 2018

Contributor

I'm not sure how, but my documentation change seems to have caused a unit test to fail for the wolfssl debug build:

test 1455...[HTTP GET when PROXY Protocol enabled]
1455: protocol FAILED:
--- log/check-expected 2018-06-11 23:04:56.700595598 +0000
+++ log/check-generated 2018-06-11 23:04:56.700595598 +0000
@@ -1,5 +0,0 @@

Is this known flakiness, or have I broken something clever?

Contributor

RobertPragSymc commented Jun 12, 2018

I'm not sure how, but my documentation change seems to have caused a unit test to fail for the wolfssl debug build:

test 1455...[HTTP GET when PROXY Protocol enabled]
1455: protocol FAILED:
--- log/check-expected 2018-06-11 23:04:56.700595598 +0000
+++ log/check-generated 2018-06-11 23:04:56.700595598 +0000
@@ -1,5 +0,0 @@

Is this known flakiness, or have I broken something clever?

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Jun 12, 2018

Member

Not your fault. 1455 has been flaky recently! :-(

Member

bagder commented Jun 12, 2018

Not your fault. 1455 has been flaky recently! :-(

@bagder bagder closed this in 9aefbff Jun 12, 2018

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Jun 12, 2018

Member

Thanks, landed. I took the liberty of changing the name in your commit so that you will end up getting credited correctly. I hope I didn't overstep anything...

Member

bagder commented Jun 12, 2018

Thanks, landed. I took the liberty of changing the name in your commit so that you will end up getting credited correctly. I hope I didn't overstep anything...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment