Curve customization support: extract curve array from yaml and make a list of mapped number strings #721

Gautham-coder · 2024-07-30T09:40:17Z

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: #713

This PR depends on: zowe/zowe-common-c#466

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Change in a documentation
Refactor the code
Chore, repository cleanup, updates the dependencies.
Breaking change (fix or feature that would cause existing functionality to not work as expected)

PR Checklist

Please delete options that are not relevant.

If the changes in this PR are meant for the next release / mainline, this PR targets the "staging" branch.
My code follows the style guidelines of this project (see: Contributing guideline)
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
New and existing unit tests pass locally with my changes
video or image is included if visual changes are made
Relevant update to CHANGELOG.md
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works, or describe a test method below

…s number string if curve is supported Signed-off-by: Gautham Kuppuswamy <gkuppuswamy@rocketsoftware.com>

Signed-off-by: Gautham Kuppuswamy <gkuppuswamy@rocketsoftware.com>

ifakhrutdinov · 2024-08-15T15:15:29Z

c/zss.c

+  Json *tlsConfig = NULL;
+  int tlsGetStatus = cfgGetAnyC(configmgr,ZSS_CFGNAME,&tlsConfig, 4,"zowe","network","server","tls");
+  if (tlsGetStatus) {
+    zowelog(NULL, LOG_COMP_ID_MVD_SERVER, ZOWE_LOG_INFO, "TLS is NOT configured for this ZSS\n");


Should this have a message ID?

I'm not sure on that because elsewhere is the code I can see its a similar message without any ID as such.
Thanks

ifakhrutdinov · 2024-08-15T15:17:14Z

c/zss.c

+    JsonObject *tlsConfigObject = jsonAsObject(tlsConfig);
+    Json *curveJson = jsonObjectGetPropertyValue(tlsConfigObject, "curves");
+    char *curves = NULL;
+    if(jsonIsArray(curveJson)) {


What if there is no curves in that JSON? I think you'll dereference a NULL pointer.

Yes Irek that's true, thanks for pointing that one.

c/zss.c

ifakhrutdinov · 2024-08-15T15:18:33Z

c/zss.c

@@ -1226,6 +1227,42 @@ static bool readAgentHttpsSettingsV2(ShortLivedHeap *slh,

  }

+  Json *tlsConfig = NULL;
+  int tlsGetStatus = cfgGetAnyC(configmgr,ZSS_CFGNAME,&tlsConfig, 4,"zowe","network","server","tls");


Why do you in some cases use blanks between arguments and in some cases you don't?

Thanks for pointing that out Irek, I will make correct this.

ifakhrutdinov · 2024-08-15T15:19:48Z

c/zss.c

+      JsonArray *curveArray = jsonObjectGetArray(tlsConfigObject, "curves");
+      int count = jsonArrayGetCount(curveArray);
+      int curveCharLength = 4;
+      curves = (char *)safeMalloc((sizeof(char) * curveCharLength * count)+1, "curve list");


NULL check missing?

Thanks Irek, I will correct it.

ifakhrutdinov · 2024-08-15T15:20:19Z

c/zss.c

+      JsonArray *curveArray = jsonObjectGetArray(tlsConfigObject, "curves");
+      int count = jsonArrayGetCount(curveArray);
+      int curveCharLength = 4;
+      curves = (char *)safeMalloc((sizeof(char) * curveCharLength * count)+1, "curve list");


Why are you casting this to char *?

Oh I think this is redundant, I will remove the typecast. Thanks

ifakhrutdinov · 2024-08-15T15:21:09Z

c/zss.c

+      int curveCharLength = 4;
+      curves = (char *)safeMalloc((sizeof(char) * curveCharLength * count)+1, "curve list");
+      for (int i = 0; i < count; i++) {
+        char *ianaName = jsonArrayGetString(curveArray, i);


Can this be NULL?

It is defined as a string so it can be NULL, I will add a check for this.
Thanks

ifakhrutdinov · 2024-08-15T15:24:48Z

c/zss.c

+    if(jsonIsArray(curveJson)) {
+      JsonArray *curveArray = jsonObjectGetArray(tlsConfigObject, "curves");
+      int count = jsonArrayGetCount(curveArray);
+      int curveCharLength = 4;


Make this const to indicate intention.

ifakhrutdinov · 2024-08-15T16:00:36Z

c/zss.c

+    if(jsonIsArray(curveJson)) {
+      JsonArray *curveArray = jsonObjectGetArray(tlsConfigObject, "curves");
+      int count = jsonArrayGetCount(curveArray);
+      int curveCharLength = 4;


Why is it 4? Can you tie it to some struct's field size?

We know that the length will always be 4. Its better not to put in the struct field because we are initializing the array of structs in zowe-common-c/h/tls.h , but we can make it as a constant as you recommended earlier.

ifakhrutdinov · 2024-08-15T16:01:13Z

c/zss.c

+        bool found = false;
+        while (curve->groupId != NULL) {
+          if (!strcmp(ianaName, curve->name)) {
+            strcat(curves, curve->groupId);


How do you know groupId is exactly 4?

Here: (https://www.ibm.com/docs/en/zos/3.1.0?topic=reference-gsk-attribute-set-buffer)

It says 1 or more 4-character decimal values.

Signed-off-by: Gautham Kuppuswamy <gkuppuswamy@rocketsoftware.com>

ifakhrutdinov self-requested a review July 31, 2024 15:25

Gautham-coder force-pushed the curve-customizaion branch from 6c1643f to 35238be Compare August 7, 2024 13:33

Gautham-coder changed the title ~~DO NOT MERGE: Extract curve array from yaml and make a list of mapped number strings~~ Curve customization support, extract curve array from yaml and make a list of mapped number strings Aug 7, 2024

Gautham-coder changed the title ~~Curve customization support, extract curve array from yaml and make a list of mapped number strings~~ Curve customization support: extract curve array from yaml and make a list of mapped number strings Aug 7, 2024

Extract curve from 'zowe.network.server.tls.curves' and make he curve…

d5af560

…s number string if curve is supported Signed-off-by: Gautham Kuppuswamy <gkuppuswamy@rocketsoftware.com>

Gautham-coder force-pushed the curve-customizaion branch from 2913004 to d5af560 Compare August 7, 2024 19:55

Updated zowe-common-c reference with latest changes

d9d7f85

Signed-off-by: Gautham Kuppuswamy <gkuppuswamy@rocketsoftware.com>

ifakhrutdinov requested changes Aug 15, 2024

View reviewed changes

Adding null checks and minor corrections related to curve processing

2abb44f

Signed-off-by: Gautham Kuppuswamy <gkuppuswamy@rocketsoftware.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Curve customization support: extract curve array from yaml and make a list of mapped number strings #721

Curve customization support: extract curve array from yaml and make a list of mapped number strings #721

Gautham-coder commented Jul 30, 2024 •

edited

Loading

ifakhrutdinov Aug 15, 2024

Gautham-coder Aug 16, 2024

ifakhrutdinov Aug 15, 2024

Gautham-coder Aug 16, 2024

ifakhrutdinov Aug 15, 2024

Gautham-coder Aug 16, 2024

ifakhrutdinov Aug 15, 2024

Gautham-coder Aug 16, 2024

ifakhrutdinov Aug 15, 2024

Gautham-coder Aug 16, 2024

ifakhrutdinov Aug 15, 2024

Gautham-coder Aug 16, 2024

ifakhrutdinov Aug 15, 2024

Gautham-coder Aug 16, 2024

ifakhrutdinov Aug 15, 2024

Gautham-coder Aug 16, 2024

ifakhrutdinov Aug 15, 2024

Gautham-coder Aug 16, 2024

Curve customization support: extract curve array from yaml and make a list of mapped number strings #721

Are you sure you want to change the base?

Curve customization support: extract curve array from yaml and make a list of mapped number strings #721

Conversation

Gautham-coder commented Jul 30, 2024 • edited Loading

Type of change

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gautham-coder commented Jul 30, 2024 •

edited

Loading