Client Certificate Authentication with R_usermap #371

jordanfilteau1995 · 2023-04-05T18:49:50Z

Client certificate is grabbed from gsk after connection is established.

Signed-off-by: Jordan Filteau <[email protected]>

ifakhrutdinov · 2023-04-18T11:24:07Z

c/tls.c

@@ -109,8 +164,7 @@ int tlsSocketInit(TlsEnvironment *env, TlsSocket **outSocket, int fd, bool isSer
  if (label) {
    rc = rc || gsk_attribute_set_buffer(socket->socketHandle, GSK_KEYRING_LABEL, label, 0);
  }
-  rc = rc || gsk_attribute_set_enum(socket->socketHandle, GSK_SESSION_TYPE,
-                                    isServer ? GSK_SERVER_SESSION : GSK_CLIENT_SESSION);
+  rc = rc || gsk_attribute_set_enum(socket->socketHandle, GSK_SESSION_TYPE, isServer ? GSK_SERVER_SESSION_WITH_CL_AUTH : GSK_CLIENT_SESSION);


Will we never be able to use this without a client certificate on the server side? Asking because it's either GSK_SERVER_SESSION_WITH_CL_AUTH or GSK_CLIENT_SESSION.

I can see cases where one would want to use this code just for a secure connection but without client authentication required.

From my understanding and testing, it just allows client authentication to be an option if a client certificate is attached. It behaves the same way as usual if you don't provide a client certificate.

ifakhrutdinov · 2023-04-18T11:44:16Z

c/tls.c

@@ -123,6 +177,7 @@ int tlsSocketInit(TlsEnvironment *env, TlsSocket **outSocket, int fd, bool isSer
    safeFree((char*)socket, sizeof(*socket));
    *outSocket = NULL;
  }
+  //printf("tlsSocketInit - rc=%d\n", rc);


No need for this.

ifakhrutdinov · 2023-04-18T11:48:46Z

c/tls.c

+
+  gsk_cert_data_elem *tmp = gskCertificateArray;
+
+  for (int i = 0; i++ < gskCertificateArrayElementCount; tmp++) {


I would not increment in a condition; this seems error prone.

What about this?

for (int i = 0; i < gskCertificateArrayElementCount; i++) { const gsk_cert_data_elem *elem = &gskCertificateArray[i]; . . . }

ifakhrutdinov · 2023-04-18T12:13:00Z

@jordanfilteau1995 , please add a detailed description of these changes. I can see at least one change which isn't related to certificates. Hoe does the security logic changes?

ifakhrutdinov

I'll let Jordan add a description before continuing with the review. For now, see my current comments.

ifakhrutdinov · 2023-04-18T11:56:32Z

c/tls.c

@@ -15,6 +15,40 @@
 #include "fdpoll.h"
 #include "tls.h"

+int getClientCertificate(gsk_handle soc_handle, char *clientCertificate, unsigned int clientCertificateBufferSize, unsigned int *clientCertificateLength) {
+
+  if (clientCertificate == NULL || clientCertificateBufferSize <= 0) {


Nit: clientCertificateBufferSize can't be less than 0.

ifakhrutdinov · 2023-04-18T11:58:37Z

c/tls.c

+    if (tmp->cert_data_id == CERT_BODY_DER) {
+      if (clientCertificateBufferSize >= tmp->cert_data_l) {
+        memcpy(clientCertificate, tmp->cert_data_p, tmp->cert_data_l);
+        *clientCertificateLength = tmp->cert_data_l;


At this point we've copied the data and continue. Is it okay to overwrite it if one of the next array entries matches the condition or do we need to break here?

ifakhrutdinov · 2023-04-18T12:00:40Z

c/tls.c

@@ -43,13 +94,15 @@ int tlsInit(TlsEnvironment **outEnv, TlsSettings *settings) {
    safeFree((char*)env, sizeof(*env));
    *outEnv = NULL;
  }
+  //printf("tlsInit - rc=%d\n", rc);


Please clean up dead code.

ifakhrutdinov · 2023-04-18T12:01:41Z

c/tls.c

+#endif
+
+  /*
+   * Only allow requests with client certificates, maybe put behind a different ifdef.


ifakhrutdinov · 2023-04-18T12:01:46Z

c/tls.c

+  /*
+   * Don't validate certificates, maybe put behind a dangerous ifdef.
+   *
+   * rc = rc || gsk_attribute_set_enum(env->envHandle, GSK_CLIENT_AUTH_TYPE, GSK_CLIENT_AUTH_PASSTHRU_TYPE);


ifakhrutdinov · 2023-04-18T12:11:27Z

c/httpserver.c

+      printf("SAF auth for user: '%s'\n", request->username);
 #else
-    printf("u: '%s' p: '%s'\n",request->username,request->password);
+      printf("u: '%s' p: '%s'\n",request->username,request->password);
 #endif
 #endif
-    if (isLowerCasePasswordAllowed() || isPassPhrase(request->password)) {
-      zowelog(NULL, LOG_COMP_HTTPSERVER, ZOWE_LOG_DEBUG3, "mixed-case system or a pass phrase, not upfolding password\n");
-      /* don't upfold password */
-    } else {
-      zowelog(NULL, LOG_COMP_HTTPSERVER, ZOWE_LOG_DEBUG3, "non-mixed-case system, not a pass phrase, upfolding password\n");
-      strupcase(request->password); /* upfold password */
+      if (isLowerCasePasswordAllowed() || isPassPhrase(request->password)) {
+        zowelog(NULL, LOG_COMP_HTTPSERVER, ZOWE_LOG_DEBUG3, "mixed-case system or a pass phrase, not upfolding password\n");
+        /* don't upfold password */
+      } else {
+        zowelog(NULL, LOG_COMP_HTTPSERVER, ZOWE_LOG_DEBUG3, "non-mixed-case system, not a pass phrase, upfolding password\n");
+        strupcase(request->password); /* upfold password */
+      }


Do we need these white space changes?

ifakhrutdinov · 2023-04-18T12:13:34Z

c/httpserver.c

+    } else {
+      pwdCheckRC = zisCheckUsername(privilegedServerName,
+          request->username, &status);
+      authResponse->type = AUTH_TYPE_RACF;
+      authResponse->responseDetails.safStatus = status.safStatus;
+    }


I need more info about this and why we need this.

Irek, we need either a new API, or a new arg in the existing API to say if password is required.

ifakhrutdinov · 2023-04-18T12:15:01Z

c/httpserver.c

+    } 
+  }
+
+  /* Doubtful that it would be greater than 8k... */


Can we know for sure? I.e. references to standards/docs would help here.

Check the R_usermap doc for limits. Some 'net searching yielded 8K, blame me for this.

ifakhrutdinov · 2023-04-18T12:35:00Z

c/httpserver.c

@@ -2653,19 +2653,23 @@ static int safAuthenticate(HttpService *service, HttpRequest *request, AuthRespo
  } else if (authDataFound){
    ACEE *acee = NULL;
    strupcase(request->username); /* upfold username */
+    if (request->password == NULL) {


Using the password field to indicate whether this is a passwordless auth seems like a bad idea. If for some unexpected reason that field is NULL (is it NULL if a user just doesn't provide a password in the basic auth?), you'll assume this is just a passwordless auth and will use zisCheckUsername, which might say everything is okay.

This seems like an error prone solution with implicit logic.

Regardless of whether this approach with client certificates is correct in the first place, using a separate variable to explicitly indicate what needs to be checked to authenticate would probably be better.

JoeNemo · 2023-04-19T15:35:34Z

@jordanfilteau1995 , can we discuss a little more. We probaby need to refactor for clarity because the different authentication methods are a little to close in the code, and could share variables and state in unexpected ways. We also may need early help in testing multiple auth methods on a branch before merging.

…dan-branch

…ls.c; simplying logic for loop in tls.c Signed-off-by: Jordan Filteau <[email protected]>

Signed-off-by: Jordan Filteau <[email protected]>

JoeNemo

Approved

JoeNemo · 2023-04-26T15:15:19Z

c/httpserver.c

+    } else {
+      pwdCheckRC = zisCheckUsername(privilegedServerName,
+          request->username, &status);
+      authResponse->type = AUTH_TYPE_RACF;
+      authResponse->responseDetails.safStatus = status.safStatus;
+    }


Irek, we need either a new API, or a new arg in the existing API to say if password is required.

JoeNemo · 2023-04-26T15:16:33Z

c/httpserver.c

+    } 
+  }
+
+  /* Doubtful that it would be greater than 8k... */


Check the R_usermap doc for limits. Some 'net searching yielded 8K, blame me for this.

jordanfilteau1995 added 2 commits April 5, 2023 11:25

testing certificate auth

c0da63b

Signed-off-by: Jordan Filteau <[email protected]>

working certificate based authentication with R_usermap.

ee4c918

Signed-off-by: Jordan Filteau <[email protected]>

jordanfilteau1995 requested review from JoeNemo and 1000TurquoisePogs April 5, 2023 19:15

grabbing cert from tls handshake

0d82a1c

Signed-off-by: Jordan Filteau <[email protected]>

jordanfilteau1995 changed the title ~~[WIP] Client Certificate Authentication with R_usermap~~ Client Certificate Authentication with R_usermap Apr 10, 2023

reverting unintentional changes

31fe0d3

Signed-off-by: Jordan Filteau <[email protected]>

ifakhrutdinov self-requested a review April 12, 2023 15:22

ifakhrutdinov reviewed Apr 18, 2023

View reviewed changes

ifakhrutdinov requested a review from rocketjared April 18, 2023 12:36

jordanfilteau1995 added 3 commits August 10, 2023 16:38

Merge branch 'v2.x/staging' of github.com:zowe/zowe-common-c into Jor…

1c66746

…dan-branch

using a flag to determine type of saf verify; removing dead code in t…

4380270

…ls.c; simplying logic for loop in tls.c Signed-off-by: Jordan Filteau <[email protected]>

using a bigger buffer for client certificate.

28accac

Signed-off-by: Jordan Filteau <[email protected]>

JoeNemo approved these changes Aug 11, 2023

View reviewed changes

JoeNemo merged commit 18cd519 into v2.x/staging Aug 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Client Certificate Authentication with R_usermap #371

Client Certificate Authentication with R_usermap #371

jordanfilteau1995 commented Apr 5, 2023 •

edited

Loading

ifakhrutdinov Apr 18, 2023 •

edited

Loading

jordanfilteau1995 Apr 26, 2023

ifakhrutdinov Apr 18, 2023

ifakhrutdinov Apr 18, 2023 •

edited

Loading

ifakhrutdinov commented Apr 18, 2023

ifakhrutdinov left a comment

ifakhrutdinov Apr 18, 2023

ifakhrutdinov Apr 18, 2023

ifakhrutdinov Apr 18, 2023

ifakhrutdinov Apr 18, 2023

ifakhrutdinov Apr 18, 2023

ifakhrutdinov Apr 18, 2023

ifakhrutdinov Apr 18, 2023

JoeNemo Apr 26, 2023

ifakhrutdinov Apr 18, 2023

JoeNemo Apr 26, 2023

ifakhrutdinov Apr 18, 2023

JoeNemo commented Apr 19, 2023

JoeNemo left a comment

JoeNemo Apr 26, 2023

JoeNemo Apr 26, 2023


		gsk_cert_data_elem *tmp = gskCertificateArray;

		for (int i = 0; i++ < gskCertificateArrayElementCount; tmp++) {

Client Certificate Authentication with R_usermap #371

Client Certificate Authentication with R_usermap #371

Conversation

jordanfilteau1995 commented Apr 5, 2023 • edited Loading

ifakhrutdinov Apr 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ifakhrutdinov Apr 18, 2023 • edited Loading

Choose a reason for hiding this comment

ifakhrutdinov commented Apr 18, 2023

ifakhrutdinov left a 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

JoeNemo commented Apr 19, 2023

JoeNemo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordanfilteau1995 commented Apr 5, 2023 •

edited

Loading

ifakhrutdinov Apr 18, 2023 •

edited

Loading

ifakhrutdinov Apr 18, 2023 •

edited

Loading