Fix wrong using of certificates in ZAAS client #1514

pj892031 · 2021-06-03T21:53:59Z

Signed-off-by: Pavel Jareš pavel.jares@broadcom.com

Description

This PR changes the setting of HTTP clients in ZAAS client. It uses now two different clients:

for passticket endpoint '/ticket' with signing by a certificate
for the rest of the endpoints (login, query, logout) without keyring in configuration

Change also required to move verification about settings of keyring to another class. Therefore few tests should be moved and fixed.

It will be great to put In the right documentation warning about using ZAAS client (but I am not sure where). ZAAS client before this PR should be used only against Gateway with disabled client certificates and each application using ZAAS client should be upgraded as soon as possible to avoid this issue.

Linked to #1513 (issue)

Type of change

Please delete options that are not relevant.

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

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

For more details about how should the code look like read the Contributing guideline

Signed-off-by: Pavel Jareš <pavel.jares@broadcom.com>

zowe-robot · 2021-06-03T21:55:27Z

The changes to the error code documentation are available in this PR: zowe/docs-site#1699

plavjanik · 2021-06-03T22:15:25Z

What happens when the ZaasClient.ticket() will be called from an application that has a valid certificate?

plavjanik · 2021-06-03T22:20:16Z

All the current users of ZAAS client are affected. If they are calling login() or query() there will be problems when client cert authentication is enabled in Zowe.

Is there an alternative solution that would not require all such users of ZaasClient to be updated?

pj892031 · 2021-06-04T07:09:30Z

What happens when the ZaasClient.ticket() will be called from an application that has a valid certificate?

The ticket call is without any change. It uses the same client as before (the same configuration).

see:

        tokens = new ZaasJwtService(httpClientProviderWithoutCert, baseUrl); // updated configuration - without keyStore* values
        passTickets = new PassTicketServiceImpl(httpClientProvider, baseUrl); // same configuration like before PR

pj892031 · 2021-06-04T07:18:02Z

All the current users of ZAAS client are affected. If they are calling login() or query() there will be problems when client cert authentication is enabled in Zowe.

Is there an alternative solution that would not require all such users of ZaasClient to be updated?

Workaround what I have in the mind:

If a service does not need a private certificate (using HTTP or ATTLS), keystore could empty, then it will be also not signed
- it is working just for the base method and possible when passticket is not used
When the private key will be not accepted by APIML, all requests will be not authorized - this is probably not a solution.
It is possible to override creating of bean (via a configuration bean) and provide an empty Keystore, but it requires also code change as upgrading of the library (maybe possible before the new release of ZAAS client)
if ZAAS client is working in HTTP mode

I guess, unfortunately, no ideas above are better than the upgrade of ZAAS client.

plavjanik · 2021-06-04T07:53:09Z

All the current users of ZAAS client are affected. If they are calling login() or query() there will be problems when client cert authentication is enabled in Zowe.
Is there an alternative solution that would not require all such users of ZaasClient to be updated?

Workaround what I have in the mind:

If a service does not need a private certificate (using HTTP or ATTLS), keystore could empty, then it will be also not signed

it is working just for the base method and possible when passticket is not used

When the private key will be not accepted by APIML, all requests will be not authorized - this is probably not a solution.

It is possible to override creating of bean (via a configuration bean) and provide an empty Keystore, but it requires also code change as upgrading of the library (maybe possible before the new release of ZAAS client)

if ZAAS client is working in HTTP mode

I guess, unfortunately, no ideas above are better than the upgrade of ZAAS client.

You are right, upgrade of the ZaasClient seems better than the alternative above.

What about following approach that will need a change in API ML only?

ZAAS server endpoints will prefer basic auth (/login) or JWT (/query, /ticket/, /logout) over the certificate provided by ZAAS client

plavjanik · 2021-06-04T07:56:55Z

What happens when the ZaasClient.ticket() will be called from an application that has a valid certificate?

The ticket call is without any change. It uses the same client as before (the same configuration).

see:
        tokens = new ZaasJwtService(httpClientProviderWithoutCert, baseUrl); // updated configuration - without keyStore* values
        passTickets = new PassTicketServiceImpl(httpClientProvider, baseUrl); // same configuration like before PR

So it means that the ticket will not work after this fix? Does X509 authentication has higher priority for the /ticket as well?

pj892031 · 2021-06-04T07:59:14Z

All the current users of ZAAS client are affected. If they are calling login() or query() there will be problems when client cert authentication is enabled in Zowe.
Is there an alternative solution that would not require all such users of ZaasClient to be updated?

Workaround what I have in the mind:

If a service does not need a private certificate (using HTTP or ATTLS), keystore could empty, then it will be also not signed

it is working just for the base method and possible when passticket is not used

When the private key will be not accepted by APIML, all requests will be not authorized - this is probably not a solution.

It is possible to override creating of bean (via a configuration bean) and provide an empty Keystore, but it requires also code change as upgrading of the library (maybe possible before the new release of ZAAS client)

if ZAAS client is working in HTTP mode

I guess, unfortunately, no ideas above are better than the upgrade of ZAAS client.

You are right, upgrade of the ZaasClient seems better than the alternative above.

What about following approach that will need a change in API ML only?

ZAAS server endpoints will prefer basic auth (/login) or JWT (/query, /ticket/, /logout) over the certificate provided by ZAAS client

I am not an APIML expert, but I guess /login endpoint is the most important for this issue and it seems Gateway has a feature that allows obtaining JWT with a signed request. It can be implemented via ignoring /api/v1/gateway/auth/login in X509Filter. I guess it requires a change in the filter to allow set this exception and then it should be probably enabled via a feature flag. @jandadav , what do you think about this solution?

plavjanik · 2021-06-04T08:02:48Z

I mean that API ML has introduced a breaking change with the X509 authentication. The applications (users of ZaasClient) that behave correctly (sending client certificate is correct for ticket endpoint and not forbidden for other endpoints) will not work.

achmelo

Code has low test coverage(not all because of these changes), I can help with that if you have a lot of work. Thanks.

achmelo · 2021-06-04T10:34:27Z

zaas-client/src/main/java/org/zowe/apiml/zaasclient/service/internal/ZaasClientImpl.java

    private final PassTicketService passTickets;

    public ZaasClientImpl(ConfigProperties configProperties) throws ZaasConfigurationException {
+        if (configProperties.getKeyStorePath() == null) {


It means that you have to provide keystore path also for Http client?

You are right, this is not complete condition. I have fixed it.

achmelo · 2021-06-04T11:13:39Z

zaas-client/src/main/java/org/zowe/apiml/zaasclient/service/internal/ZaasClientImpl.java

+        }
+
        CloseableClientProvider httpClientProvider = getTokenProvider(configProperties);
+        CloseableClientProvider httpClientProviderWithoutCert = getTokenProviderWithoutCert(configProperties, httpClientProvider);


In case of HTTP , the same provider object is used, in case of HTTPS, there are 2 objects being created, is this intentional?

Yes, it is, in the case of HTTP there is no needed to make two instances. But the purpose is the question because HTTP is not possible to use (except the theoretical configuration). It should be just for ATTLS, but then it is too difficult (maybe impossible) to establish to respect URL and using a client certificate.

We should focus on the possibility, how to use ZAAS client with AT-TLS in the next issue. Thank you for finding that.

achmelo · 2021-06-04T11:19:26Z

zaas-client/src/main/java/org/zowe/apiml/zaasclient/config/ConfigProperties.java

    private boolean nonStrictVerifySslCertificatesOfServices;
+
+    @Tolerate
+    public ConfigProperties() {


Did you consider using NoArgs and AllArgs annotations instead of this experimental one?

It was my first attempt, but it doesn't work together with @builder. Just this annotation solved it.

Signed-off-by: Pavel Jareš <pavel.jares@broadcom.com>

sonarqubecloud · 2021-06-04T12:20:45Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

48.3% Coverage
0.0% Duplication

jandadav · 2021-06-09T07:08:57Z

@achmelo is the review complete? If we want to release this, we should merge before Friday. Thanks!

fix

9ceae38

Signed-off-by: Pavel Jareš <pavel.jares@broadcom.com>

pj892031 requested review from plavjanik and jandadav June 3, 2021 21:54

zowe-robot added the docs Related to creation of new doc(s), or editing existing doc(s) label Jun 3, 2021

pj892031 linked an issue Jun 3, 2021 that may be closed by this pull request

Wrong using of certificates in ZAAS client #1513

Closed

achmelo reviewed Jun 4, 2021

View reviewed changes

fix by review

c253636

Signed-off-by: Pavel Jareš <pavel.jares@broadcom.com>

achmelo approved these changes Jun 9, 2021

View reviewed changes

achmelo merged commit 964c4fa into master Jun 9, 2021

delete-merged-branch bot deleted the lpl/fixZaasClientCertsOnCall branch June 9, 2021 08:21

Fix wrong using of certificates in ZAAS client #1514

Fix wrong using of certificates in ZAAS client #1514

Uh oh!

Conversation

pj892031 commented Jun 3, 2021

Description

Type of change

Checklist:

Uh oh!

zowe-robot commented Jun 3, 2021

Uh oh!

plavjanik commented Jun 3, 2021

Uh oh!

plavjanik commented Jun 3, 2021

Uh oh!

pj892031 commented Jun 4, 2021

Uh oh!

pj892031 commented Jun 4, 2021

Uh oh!

plavjanik commented Jun 4, 2021

Uh oh!

plavjanik commented Jun 4, 2021

Uh oh!

pj892031 commented Jun 4, 2021

Uh oh!

plavjanik commented Jun 4, 2021

Uh oh!

achmelo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achmelo Jun 4, 2021

Choose a reason for hiding this comment

Uh oh!

pj892031 Jun 4, 2021

Choose a reason for hiding this comment

Uh oh!

achmelo Jun 4, 2021

Choose a reason for hiding this comment

Uh oh!

pj892031 Jun 4, 2021

Choose a reason for hiding this comment

Uh oh!

pj892031 Jun 4, 2021

Choose a reason for hiding this comment

Uh oh!

achmelo Jun 4, 2021

Choose a reason for hiding this comment

Uh oh!

pj892031 Jun 4, 2021

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Jun 4, 2021

Uh oh!

jandadav commented Jun 9, 2021

Uh oh!

Uh oh!

achmelo left a comment •

edited

Loading