Curve customization support: Curve definitions and setting them using gsk_attribute_set_buffer #466
Conversation
Signed-off-by: Gautham Kuppuswamy <[email protected]>
Gautham-coder
force-pushed
the
test-curve-customization
branch
from
3c6ed63
to
8737e75
Compare
Gautham-coder
changed the title
Signed-off-by: Gautham Kuppuswamy <[email protected]>
|
What is expected behavior when |
|
At present, the default settings is, at-tls is set to True. |
| Currently, only the curves mentioned https://www.ibm.com/docs/en/zos/3.1.0?topic=programming-cipher-suite-definitions#csdcwh__tttcsd | ||
| are supported, if any curves are added in the future they should be added to the above array. | ||
|
|
||
| {"NIST K-163", TLS_CURVE_SECT163K1},\ |
There was a problem hiding this comment.
Why don't we use all these?
| char *curves = settings->curves; | ||
| int curveRC = 0; | ||
| if (curves) { | ||
| curveRC = gsk_attribute_set_buffer(env->envHandle, GSK_CLIENT_ECURVE_LIST, curves, 0); |
There was a problem hiding this comment.
Do we need to set this if this is set in tlsSocketInit? What's the purpose?
| if (!curveRC) { | ||
| zowelog(NULL, LOG_COMP_HTTPSERVER, ZOWE_LOG_DEBUG, "Curves set using gsk_attribute_set_buffer\n"); | ||
| } else { | ||
| zowelog(NULL, LOG_COMP_HTTPSERVER, ZOWE_LOG_DEBUG, "Failure to set curves using gsk_attribute_set_buffer\n"); |
There was a problem hiding this comment.
Since this is a debug message and it doesn't need to be super nice, can you just have a single message which reports the RC value?
h/tls.h
Outdated
| @@ -244,6 +279,75 @@ typedef struct CipherMap_tag { | |||
| {0, NULL}\ | |||
| }; | |||
|
|
|||
| typedef struct CurveMap_tag { | |||
| const char* name; | |||
| const char* groupId; //number string | |||
There was a problem hiding this comment.
number string is very generic, what does it even mean? Can you be more specific, for example, I.A.N.A Elliptic curve enumerator?
| @@ -236,6 +248,17 @@ int tlsSocketInit(TlsEnvironment *env, TlsSocket **outSocket, int fd, bool isSer | |||
| rc = rc || gsk_attribute_set_buffer(socket->socketHandle, GSK_V3_CIPHER_SPECS_EXPANDED, ciphers, 0); | |||
| rc = rc || gsk_attribute_set_enum(socket->socketHandle, GSK_V3_CIPHERS, GSK_V3_CIPHERS_CHAR4); | |||
| } | |||
| char *curves = env->settings->curves; | |||
| int curveRC = 0; | |||
There was a problem hiding this comment.
Why declare it here, do you need it later?
Signed-off-by: Gautham Kuppuswamy <[email protected]>
Proposed changes
'zowe.network.server.tls.curves' is an array a user can set in zowe.yaml to customize crypto curves.
GSK handles curves as a string of 4 digit numbers(IANA numbers) back to back without any spaces or symbols in between.
Its very unfriendly to a human, so a mapping from names to numbers is needed, this is done in tls.h, for now only supported curves are in the array. Unsupported curves are commented and can be moved into this array as and when the supported curves are updated.
Curves are set during TLS settings initialization using gsk_attribute_set_buffer(), using 'GSK_CLIENT_ECURVE_LIST', see here
https://www.ibm.com/docs/en/zos/3.1.0?topic=reference-gsk-attribute-set-buffer for reference,
Currently, the supported curves are here, https://www.ibm.com/docs/en/zos/3.1.0?topic=programming-cipher-suite-definitions#csdcwh__tttcsd
Testing:
The curves that are not supported show an error in gsktrace as below, this was tested by adding some unsupported curves into the curve map array.
ERROR set_binary_ecurves(): Elliptical curve 0001 not supported
ERROR set_binary_ecurves(): Elliptical curve 0009 not supported
ERROR set_binary_ecurves(): Elliptical curve 0026 not supported
To show the error in normal logs, only valid curves are in the mapping array. So if any invalid curve is mentioned in zowe.yaml an invalid curve message is logged.
zowe.network.server.tls.curves: ["x25519", "x448", "secp192r1", "secp224r1","prime256v1","secp384r1", "secp521r1"]
,
is converted to number string 0029003000190021002300240025
This PR addresses Issue: zowe/zss#713
Type of change
Please delete options that are not relevant.
PR Checklist
Please delete options that are not relevant.