Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed ZMSUtils to correctly determine PrincipalType #2556

Merged
merged 7 commits into from
Mar 22, 2024

Conversation

hiragi-gkuth
Copy link
Contributor

Description

Background is similar to #2532.
When home domain and user domain are identical, the ZMSUtils.principalType() returns Principal.Type.USER regardless a provided principal string was Principal.Type.GROUP.

e.g.
home domain = user domain = personal

ZMSUtils.principalType("personal.hiragi-gkuth") // returns USER
ZMSUtils.principalType("personal.hiragi-gkuth.service") // returns USER but SERVICE is expected
ZMSUtils.principalType("personal.hiragi-gkuth:group.mygroup") // returns USER but GROUP is expected
ZMSUtils.principalType("personal.hiragi-gkuth.subdomain:group.subgroup") // returns USER but GROUP is expected

So we add more conditions.

Contribution Checklist:

  • The pull request does not introduce any breaking changes
    • There might be unintended consequences that I haven't anticipated.
  • I have read the contribution guidelines.
  • Create an issue and link to the pull request.

Attach Screenshots (Optional)

hiragi-gkuth and others added 4 commits March 7, 2024 14:06
Signed-off-by: Shimaoka Shuya <sshimaok@lycorp.co.jp>
Signed-off-by: Shimaoka Shuya <sshimaok@lycorp.co.jp>
Signed-off-by: Shimaoka Shuya <sshimaok@lycorp.co.jp>
@hiragi-gkuth
Copy link
Contributor Author

I'll fix failed test.

@hiragi-gkuth
Copy link
Contributor Author

hiragi-gkuth commented Mar 19, 2024

I have a few questions regarding this test code.

After adding some conditions to determine the principal type,these lines started to fail. I had thought that user.jack.sub1.api was not a valid principal name for the Athenz Service (but home.jack.sub1.api is valid I think). Is this correct? And what is the purpose of adding user.jack.sub1.api to the Athenz Group in this test case?

additional note:
I found that ZMSTestInitializer doesn't set prefixes, which results in the user domain and home domain being the same.

public void setUp() throws Exception {
MockitoAnnotations.openMocks(this);
System.setProperty(ZMSConsts.ZMS_PROP_OBJECT_STORE_FACTORY_CLASS, "com.yahoo.athenz.zms.store.impl.JDBCObjectStoreFactory");
System.setProperty(ZMSConsts.ZMS_PROP_JDBC_RW_STORE, mysqld.getJdbcUrl());
System.setProperty(ZMSConsts.ZMS_PROP_JDBC_RW_USER, DB_USER);
System.setProperty(ZMSConsts.ZMS_PROP_JDBC_RW_PASSWORD, DB_PASS);
when(mockServletRequest.getRemoteAddr()).thenReturn(MOCKCLIENTADDR);
when(mockServletRequest.isSecure()).thenReturn(true);
when(mockServletRequest.getRequestURI()).thenReturn("/zms/v1/request");
when(mockServletRequest.getMethod()).thenReturn("GET");
System.setProperty(ZMSConsts.ZMS_PROP_FILE_NAME, "src/test/resources/zms.properties");
System.setProperty(ZMSConsts.ZMS_PROP_METRIC_FACTORY_CLASS, METRIC_DEFAULT_FACTORY_CLASS);
System.setProperty(ZMSConsts.ZMS_PROP_PROVIDER_ENDPOINTS, ".athenzcompany.com");
System.setProperty(ZMSConsts.ZMS_PROP_MASTER_COPY_FOR_SIGNED_DOMAINS, "true");
System.setProperty(ZMSConsts.ZMS_PROP_PRIVATE_KEY_STORE_FACTORY_CLASS,
"com.yahoo.athenz.auth.impl.FilePrivateKeyStoreFactory");
System.setProperty(FilePrivateKeyStore.ATHENZ_PROP_PRIVATE_KEY, "src/test/resources/unit_test_zms_private.pem");
System.setProperty(ZMS_PROP_PUBLIC_KEY, "src/test/resources/zms_public.pem");
System.setProperty(ZMSConsts.ZMS_PROP_DOMAIN_ADMIN, "user.testadminuser");
System.setProperty(ZMSConsts.ZMS_PROP_AUTHZ_SERVICE_FNAME,
"src/test/resources/authorized_services.json");
System.setProperty(ZMSConsts.ZMS_PROP_SOLUTION_TEMPLATE_FNAME,
"src/test/resources/solution_templates.json");
System.setProperty(ZMSConsts.ZMS_PROP_NOAUTH_URI_LIST,
"uri1,uri2,uri3+uri4");
System.setProperty(ZMSConsts.ZMS_PROP_AUDIT_REF_CHECK_OBJECTS,
"role,group,policy,service,domain,entity,tenancy,template");
System.setProperty(ZMSConsts.ZMS_PROP_VALIDATE_ASSERTION_ROLES, "true");
System.setProperty(ZMSConsts.ZMS_PROP_PRINCIPAL_STATE_UPDATER_DISABLE_TIMER, "true");
System.setProperty(ZMSConsts.ZMS_PROP_MAX_POLICY_VERSIONS, "5");
String certPath = Resources.getResource("service.provider.cert.pem").getPath();
String keyPath = Resources.getResource("service.provider.key.pem").getPath();
System.setProperty(ZMSConsts.ZMS_PROP_PROVIDER_KEY_PATH, keyPath);
System.setProperty(ZMS_PROP_PROVIDER_CERT_PATH, certPath);
System.setProperty(ZMS_PROP_PROVIDER_TRUST_STORE, "test.truststore");
System.setProperty(ZMS_PROP_PROVIDER_TRUST_STORE_PASSWORD, "test.truststore.password");
auditLogger = new DefaultAuditLogger();
initializeZms();
}

@havetisyan
Copy link
Collaborator

If you’re using user as the prefix for your personal domains then yes, user.jack.sub1.api is a valid service principal.

@havetisyan
Copy link
Collaborator

We don’t recommend using the same prefix for users and home domains - for backwards compatibility they’re the same value but from experience it’s confusing so we have switched to using user to identify users and home to identify personal domains. So when we get user.joe it’s always a user identity and when we get home.joe then it’s user’s personal domain.

@havetisyan
Copy link
Collaborator

havetisyan commented Mar 21, 2024

add the following two blocks on line 84 in the test case and that should fix the failure.

        ServiceIdentity service2 = new ServiceIdentity().setName(ResourceUtils.serviceResourceName("user.jack.sub1", "api"));
        zmsImpl.putServiceIdentity(ctx, "user.jack.sub1", "api", auditRef, false, service2);

        ServiceIdentity service3 = new ServiceIdentity().setName(ResourceUtils.serviceResourceName("user.jack.sub1", "service"));
        zmsImpl.putServiceIdentity(ctx, "user.jack.sub1", "service", auditRef, false, service3);

hiragi-gkuth and others added 3 commits March 22, 2024 17:56
Signed-off-by: Shimaoka Shuya <sshimaok@lycorp.co.jp>
Signed-off-by: Shimaoka Shuya <sshimaok@lycorp.co.jp>
@hiragi-gkuth
Copy link
Contributor Author

add the following two blocks on line 84 in the test case and that should fix the failure.

Thank you. I realized that someone had just not created the service because the tests were being conducted with the user and home domains being the same.

@havetisyan havetisyan merged commit e9eac1e into AthenZ:master Mar 22, 2024
2 checks passed
@hiragi-gkuth hiragi-gkuth deleted the fix-principalTypeUtil branch March 25, 2024 01:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants