RBAC checking via custom authHandler function + SAF query interpretation #281

DivergentEuropeans · 2021-04-13T06:25:37Z

Similar to what zlux-server-framework\plugins\sso-auth\lib\safprofile.js does

Turns a SAF URL into a SAF query i.e. /plugins GET undefined
->
ZLUX.0.COR.GET.PLUGINS

ZSS now uses RBAC for Http services

List of exclusions:
'/login', '/logout', '/password', '/unixfile', '/datasetContents', '/VSAMdatasetContents', '/datasetMetadata', '/omvs', '/security-mgmt'

PR (1 of 2)
PR 2: zowe/zowe-common-c#218

Signed-off-by: Leanid Astrakou [email protected]

Signed-off-by: Leanid Astrakou <[email protected]>

c/authService.c

Signed-off-by: Leanid Astrakou <[email protected]>

Addressed

timgerstel

Seems okay to me

c/authService.c

Signed-off-by: Leanid Astrakou <[email protected]>

c/authService.c

Signed-off-by: Leanid Astrakou <[email protected]>

ifakhrutdinov

@DivergentEuropeans, please see my comments.

In addition to the comments:

Please add some documentation (preferably Doxygen) to the function prototypes, it helps to understand what they try to achieve. For example, https://github.com/zowe/zowe-common-c/blob/master/h/radmin.h#L520 ✔️
Consider splitting getProfileNameFromRequest, it's huge and hard to follow ✔️
You don't need to reinvent all that URL parsing, the HTTP request already has a split URL, see https://github.com/zowe/zss/blob/staging/c/securityService.c#L579. Can you use or re-use any of that? ✔️

c/authService.c

Signed-off-by: Leanid Astrakou <[email protected]>

h/authService.h

Signed-off-by: Leonty Chudinov <[email protected]>

Rbac code cleanup

Signed-off-by: Leanid Astrakou <[email protected]>

…into RBAC-support Signed-off-by: Leanid Astrakou <[email protected]>

DivergentEuropeans · 2021-08-26T05:31:01Z

TODO: When we login, the App server interprets the GET plugins query as

ZLUX.0.COR.GET.PLUGINS

yet ZSS interprets it as

ZLUX.0.COR.GET.SAF-AUTH.ZLUX.0.COR.GET.PLUGINS.READ

for some reason. Need to investigate if this is intended behaviour...

Signed-off-by: Leanid Astrakou <[email protected]>

Signed-off-by: Leonty Chudinov <[email protected]>

…inov/zss into RBAC-support

Signed-off-by: Leonty Chudinov <[email protected]>

…tion-for-saf-auth-service Disable RBAC authorization for saf-auth service

Signed-off-by: Leonty Chudinov <[email protected]>

Fix buffer size

Signed-off-by: Leanid Astrakou <[email protected]>

ifakhrutdinov · 2021-11-03T12:50:55Z

c/authService.c

+  const char *class = SAF_CLASS;
+
+  int rc = zisCheckEntity(privilegedServerName, userName, class, entity, access, &reqStatus);
+  zowelog(NULL, LOG_COMP_ID_SECURITY, ZOWE_LOG_DEBUG2,


I'd add zowedump for reqStatus so you can debug what's happening.

ifakhrutdinov · 2021-11-03T13:27:53Z

c/authService.c

+    while (pathSegment != NULL) {
+      snprintf(urlSegment, sizeof(urlSegment), "%s", pathSegment->string);
+      strupcase(urlSegment);
+      if (rootServiceName == NULL)


This can never be NULL. And if it was, you wouldn't want to call snprintf with rootServiceName as the destination.

ifakhrutdinov · 2021-11-03T13:32:45Z

c/authService.c

+        case 0:
+          snprintf(productCode, sizeof(productCode), urlSegment);
+          break;
+        case 1:
+          break;
+        case 2:
+          snprintf(pluginID, sizeof(pluginID), urlSegment);
+          break;
+        case 3:
+          break;
+        case 4:
+          snprintf(serviceName, sizeof(serviceName), urlSegment);
+          break;


If these cases are never entered, these buffers will stay uninitialized, this will cause issues in setProfileNameAttribs and makeProfileName.

ifakhrutdinov · 2021-11-03T13:36:30Z

c/authService.c

+    setProfileNameAttribs(pluginID, serviceName, type, scope, subUrl);
+    int pluginIDLen = strlen(pluginID);
+    for (int index = 0; index < pluginIDLen; index++) {
+  		if (pluginID[index] == '.') {


pluginID may be uninitialized.

ifakhrutdinov · 2021-11-03T13:36:38Z

c/authService.c

+    productCode, 
+    instanceID, 
+    pluginID, 
+    rootServiceName,


rootServiceName may be uninitialized.

ifakhrutdinov · 2021-11-03T18:16:11Z

c/datasetService.c

@@ -188,6 +188,7 @@ void installDatasetContentsService(HttpServer *server) {

  HttpService *httpService = makeGeneratedService("datasetContents", "/datasetContents/**");
  httpService->authType = SERVICE_AUTH_NATIVE_WITH_SESSION_TOKEN;
+  httpService->authorizationType = SERVICE_AUTHORIZATION_TYPE_NONE;


Do 3rd party plugins have to do this as well and then rebuild the binaries, or will everything work without recompilation?

ifakhrutdinov · 2021-11-03T18:20:18Z

c/zss.c

+  RbacAuthorizationData *rbacData = userData;
+
+  char method[16];
+  snprintf(method, sizeof(method), "%s", request->method);


Should we validate the method length and not proceed if it's too long?

ifakhrutdinov · 2021-11-03T18:23:50Z

c/zss.c

+  return rbacParm;
+}
+
+static int getZoweInstanceId() {


Declare functions that take no parameters using a void argument, for example:

int foo(void);

Otherwise it's considered "unspecified number of parameters" by the standard.

ifakhrutdinov · 2021-11-03T18:24:40Z

c/zss.c

+    return;
+  }
+  RbacAuthorizationData *rbacData = (RbacAuthorizationData*) safeMalloc(sizeof(*rbacData), "Rbac Authorization Data");
+  if (rbacData) {


Shouldn't we report an error and maybe terminate if this is NULL? If we silently do nothing here, ZSS is less protected and that's not going to be discovered.

ifakhrutdinov · 2021-11-03T18:26:36Z

h/authService.h

@@ -26,9 +26,37 @@
 #include "httpserver.h"
 #include "dataservice.h"

+#define SAF_CLASS "ZOWE"


Can you be more specific? ZOWE_SAF_CLASS would be clearer.

Signed-off-by: Leonty Chudinov <[email protected]>

Initial commit for SAF query interpretation methods

3a8e00b

Signed-off-by: Leanid Astrakou <[email protected]>

DivergentEuropeans requested review from timgerstel, 1000TurquoisePogs, ifakhrutdinov, lchudinov and a team April 13, 2021 06:25

DivergentEuropeans self-assigned this Apr 13, 2021

DivergentEuropeans added 2 commits April 13, 2021 02:27

Increase buffer size for subUrl

94742a0

Signed-off-by: Leanid Astrakou <[email protected]>

Changed buffer size for some other strings

278dc2b

Signed-off-by: Leanid Astrakou <[email protected]>

lchudinov previously requested changes Apr 13, 2021

View reviewed changes

DivergentEuropeans added 3 commits April 13, 2021 23:34

Code review comments regarding NULL & buffer size

35fe4be

Signed-off-by: Leanid Astrakou <[email protected]>

Added safeMalloc

f4fb448

Signed-off-by: Leanid Astrakou <[email protected]>

strcpy -> snprintf to avoid possible overflow

9a191ad

Signed-off-by: Leanid Astrakou <[email protected]>

DivergentEuropeans requested a review from lchudinov April 14, 2021 04:28

DivergentEuropeans added 3 commits April 14, 2021 00:31

Increased buffer for some variables

2b271d7

Signed-off-by: Leanid Astrakou <[email protected]>

sprintf --> snprintf to avoid overflow

40c0770

Signed-off-by: Leanid Astrakou <[email protected]>

Small typo

7df8380

Signed-off-by: Leanid Astrakou <[email protected]>

timgerstel reviewed Apr 14, 2021

View reviewed changes

c/authService.c Outdated Show resolved Hide resolved

Changed snprintf method usage

21eb96d

Signed-off-by: Leanid Astrakou <[email protected]>

lchudinov reviewed Apr 15, 2021

View reviewed changes

c/authService.c Outdated Show resolved Hide resolved

Revert

038b915

Signed-off-by: Leanid Astrakou <[email protected]>

ifakhrutdinov previously requested changes Apr 15, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

DivergentEuropeans added 3 commits April 24, 2021 22:02

Addressed more code review like NULL checks

0d154ad

Signed-off-by: Leanid Astrakou <[email protected]>

Some prototype RBAC checking + unfinished comments

c91b2dc

Signed-off-by: Leanid Astrakou <[email protected]>

Merge branch 'staging' of github.com:zowe/zss into RBAC-support

54a35c5

Signed-off-by: Leanid Astrakou <[email protected]>

DivergentEuropeans changed the title ~~Initial commit for SAF query interpretation methods~~ (WIP) Initial commit for SAF query interpretation + RBAC checking May 24, 2021

Annoying char change

a618f31

Signed-off-by: Leanid Astrakou <[email protected]>

DivergentEuropeans commented May 24, 2021

View reviewed changes

h/authService.h Outdated Show resolved Hide resolved

Leonty Chudinov and others added 6 commits July 7, 2021 11:14

Add profileNameBufSize arg

1a63674

Signed-off-by: Leonty Chudinov <[email protected]>

Add const for arguments

961b7a7

Signed-off-by: Leonty Chudinov <[email protected]>

Merge pull request #310 from lchudinov/rbac-code-cleanup

f774e80

Rbac code cleanup

Merge branch 'staging' of github.com:zowe/zss into RBAC-support

a4ad82a

Signed-off-by: Leanid Astrakou <[email protected]>

Updated with more of Irek's suggested code changes

f3b8d91

Signed-off-by: Leanid Astrakou <[email protected]>

Merge branch 'refactor-rbac-code' of https://github.com/lchudinov/zss …

ddf0408

…into RBAC-support Signed-off-by: Leanid Astrakou <[email protected]>

DivergentEuropeans and others added 20 commits August 26, 2021 10:39

Removed 'class' as an arg

e2ed56e

Signed-off-by: Leanid Astrakou <[email protected]>

Merge branch 'staging' of github.com:zowe/zss into RBAC-support

47a8af9

Signed-off-by: Leanid Astrakou <[email protected]>

PassTicket REST API skeleton

4fc145c

Signed-off-by: Leonty Chudinov <[email protected]>

Implement PassTicket generation

542f458

Signed-off-by: Leonty Chudinov <[email protected]>

Refactoring for PassTicket

414466c

Signed-off-by: Leonty Chudinov <[email protected]>

Add passTicketService.c to build_zss.sh

ad39d3b

Signed-off-by: Leonty Chudinov <[email protected]>

Update Changelog

3b10697

Signed-off-by: Leonty Chudinov <[email protected]>

Cleanup includes

918bf99

Signed-off-by: Leonty Chudinov <[email protected]>

Merge branch 'feature/passticket-service' of https://github.com/lchud…

6b1f6b4

…inov/zss into RBAC-support

Disable RBAC authorization for saf-auth service

b83e85e

Signed-off-by: Leonty Chudinov <[email protected]>

Update pointer to zowe-common-c

42acd8e

Signed-off-by: Leonty Chudinov <[email protected]>

Update pointer to zowe-common-c

a4b56df

Signed-off-by: Leonty Chudinov <[email protected]>

Merge pull request #331 from lchudinov/feature/disable-rbac-authoriza…

909ecb2

…tion-for-saf-auth-service Disable RBAC authorization for saf-auth service

Fix buffer size

0899ef7

Signed-off-by: Leonty Chudinov <[email protected]>

Merge pull request #332 from lchudinov/bugfix/fix-buffer-size

7c6cd42

Fix buffer size

Updated zowe-common-c

acf074c

Signed-off-by: Leanid Astrakou <[email protected]>

Merge branch 'RBAC-support' of github.com:zowe/zss into RBAC-support

46e527d

Merge branch 'staging' of github.com:zowe/zss into RBAC-support

ea23113

Signed-off-by: Leanid Astrakou <[email protected]>

Updated zowe-common-c

65e909e

Signed-off-by: Leanid Astrakou <[email protected]>

Small changelog typo

6a66374

Signed-off-by: Leanid Astrakou <[email protected]>

ifakhrutdinov reviewed Nov 3, 2021

View reviewed changes

Leonty Chudinov added 2 commits November 29, 2021 10:27

Merge remote-tracking branch 'origin/staging' into RBAC-support

47b6665

Adderss code review

8160346

Signed-off-by: Leonty Chudinov <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RBAC checking via custom authHandler function + SAF query interpretation #281

RBAC checking via custom authHandler function + SAF query interpretation #281

DivergentEuropeans commented Apr 13, 2021 •

edited

Loading

timgerstel left a comment

ifakhrutdinov left a comment •

edited by DivergentEuropeans

Loading

This comment has been minimized.

DivergentEuropeans commented Aug 26, 2021

ifakhrutdinov Nov 3, 2021

ifakhrutdinov Nov 3, 2021

ifakhrutdinov Nov 3, 2021

ifakhrutdinov Nov 3, 2021

ifakhrutdinov Nov 3, 2021

ifakhrutdinov Nov 3, 2021

ifakhrutdinov Nov 3, 2021

ifakhrutdinov Nov 3, 2021

ifakhrutdinov Nov 3, 2021

ifakhrutdinov Nov 3, 2021

RBAC checking via custom authHandler function + SAF query interpretation #281

Are you sure you want to change the base?

RBAC checking via custom authHandler function + SAF query interpretation #281

Conversation

DivergentEuropeans commented Apr 13, 2021 • edited Loading

timgerstel left a comment

Choose a reason for hiding this comment

ifakhrutdinov left a comment • edited by DivergentEuropeans Loading

Choose a reason for hiding this comment

This comment has been minimized.

DivergentEuropeans commented Aug 26, 2021

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

DivergentEuropeans commented Apr 13, 2021 •

edited

Loading

ifakhrutdinov left a comment •

edited by DivergentEuropeans

Loading