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

PAYARA-3101 MicroProfile endpoints security support #3975

Merged
merged 14 commits into from Jun 14, 2019

Conversation

@jGauravGupta
Copy link
Contributor

commented May 23, 2019

MicroProfile endpoints (/metrics, /health, and /openapi) are configurable through both the asadmin commands and admin console. Here demonstrating the endpoints security configuration via Metrics example.

To enable the security of metrics endpoint, Run the following asadmin command in your terminal:

asadmin> set-metrics-configuration --securityenabled=true 

When securityenabled is set to true, a username and password are required to access Metrics endpoint. By default, the username is set to mp and the password is set to mp with the role microprofile. The user mp is defined in the file realm (${domain}/config/keyfile).

To provide the access to the Metrics endpoint to any user, User must be defined in the file realm and be part of the microprofile group.

If the application deployed to root context then microprofile endpoints can share the security config of app.

Test PR in payara/payara-samples#12

@jGauravGupta

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

jenkins test please

@arjantijms arjantijms added this to the 5.193 milestone May 24, 2019

@arjantijms arjantijms requested review from jbee and pdudits May 27, 2019

@arjantijms

This comment has been minimized.

Copy link
Contributor

commented May 27, 2019

If the application deployed to root context then microprofile endpoints can share the security config of app.

As a clarification, the endpoints then MUST share the security config with whatever application is deployed to root, as there can be only one security configuration.

@arjantijms

This comment has been minimized.

Copy link
Contributor

commented May 27, 2019

I had rather seen independent security configuration for the endpoints, but for now it does solve the issue so approving.

@arjantijms arjantijms requested a review from rdebusscher May 27, 2019

@arjantijms

This comment has been minimized.

Copy link
Contributor

commented May 27, 2019

@rdebusscher Can you verify that the approach is okay for you? The full review (code and such) can be let to @jbee.

@jbee
Copy link
Contributor

left a comment

I'm no expert when it comes our .jsf so cannot say much on that.

Java code has some duplication I'd hope could be eliminated somewhat. Otherwise just some minor comments.

@rdebusscher

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

@rdebusscher Can you verify that the approach is okay for you?

If it is by default off, then I assume it is OK when they use the maven/gradle plugins (since they deploy app on root)

PAYARA-3101 is not only about /metrics, so I assume other PRs will be created for the other MP endpoints? @jGauravGupta

@OndroMih You worked with BMW on that ticket, what is you opinion of this solution?

@arjantijms I'll try to have a more detailed look this evening ...

@jGauravGupta

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

Hi @rdebusscher,

This PR covers all 3 endpoints /metrics, /health, and /openapi.

@rdebusscher
Copy link
Contributor

left a comment

@jGauravGupta ok, I saw only the metrics asadmin command but you did mention the other endpoints and I see them in the code.

Only minor remark about C-style array declaration that can be ignored.

@arjantijms

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

Jenkins test please

@jGauravGupta jGauravGupta force-pushed the jGauravGupta:PAYARA-3101 branch from c184edb to 4312481 May 30, 2019

@jGauravGupta

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

Jenkins test please

@OndroMih

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

The behavior described by @jGauravGupta looks good to me and it's according to what the customer expects.

I would rename the new --securityenabled argument to --authorizationrequired. The current name is confusing. There's already a --securemetrics argument, which triggers SSL encryption. We already use the word "security" in various cases, and very often it means encryption (secure admin, secure metrics, secure HTTP listener). I suggest using terms like "restricted access", "authorized access" or "authorization" for things related to allowing access only for privileged users and avoid using the word "security" in these cases.

@jGauravGupta

This comment has been minimized.

Copy link
Contributor Author

commented Jun 1, 2019

Hi @OndroMih,

I agree that with existing --securemetrics argument & the new --securityenabled argument is confusing and --authorizationRequired makes more sense. But to make consistency with other existing asadmin command like set-rest-monitoring-configuration introduced --securityenabled argument.
// @Pandrex247

@Pandrex247

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

Well for one is --securemetrics actually any different to --securityenabled? They look suspiciously similar...

And yes I agree @jGauravGupta ideally we'd keep things consistent.

Hmm, not convinced about --authorizationrequired. As you said @OndroMih securityEnabled is pretty well established in Payara as turning on HTTPS, but I'd also argue that it's fairly well established that it also makes auth required - are there any cases where you can enable HTTPS and auth separately?

Specifically making it auth required also sounds a bit "developer-y" to me: technically correct vs. common usage.

@jGauravGupta

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

Thanks, @OndroMih and @Pandrex247 for your feedback.
IMO --securemetrics argument can be marked as obsolete and --securityenabled argument can be used to turn on HTTPS and make auth required. Wdyt?

jGauravGupta added 2 commits Jun 7, 2019

@jGauravGupta jGauravGupta requested review from OndroMih, pdudits and jbee Jun 7, 2019

@jbee
jbee approved these changes Jun 10, 2019
@jGauravGupta

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

jenkins test please

@OndroMih

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

@Pandrex247

Well for one is --securemetrics actually any different to --securityenabled?

Well, your question exactly proves that there's a lot of confution in how the word "secure" is used. There's a remarkably huge difference between --securemetrics and --securityenabled, at least to how --securemetrics used to work before 5.192. While the first only required a connection over SSL, the second requires an SSL connection AND authentication.

If we agree that "secure" always means both SSL and authentication, then I'm fine with leaving the wording as is. I understand that we're deprecating the --securemetrics option. So "securing" in different contexts means:

  • secured MP metrics/health/openapi means only authorized users are allowed and only over SSL ☑️
  • secured JMX connector means it works only over SSL. The Admin console hints that client authentication needs to be enabled separately but I'm not sure how this works.
  • Secured DAS means access is allowed only over SSL and it requires authentication ☑️
  • Secure access for REST Monitoring means authentication is required. It is bound to the admin listener so SSL is configured separately. ☑️

It seems that all contexts I found are more or less in line with this definition of "secure". However, I still recommend to use the word "restrict access" instead of just "secure" internally for new features to ensure that it's also possible to require authentication and not only SSL encryption.

@Pandrex247 Pandrex247 merged commit 1a0fb71 into payara:master Jun 14, 2019

59 checks passed

Payara Quick Build and Test Quick build and test passed!
Details
security/snyk - api/payara-api/pom.xml (payara-ci) No new issues
Details
security/snyk - api/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/admin/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/admingui/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ant-tasks/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/appclient/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/batch/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/common/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/concurrent/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/connectors/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/core/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/deployment/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/distributions/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ejb/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/extras/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/featuresets/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/flashlight/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/grizzly/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ha/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/installer/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/javaee-api/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/jdbc/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/jms/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/load-balancer/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/orb/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/osgi-platforms/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/packager/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/payara-appserver-modules/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/persistence/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/registration/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/resources/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/security/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/tests/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/transaction/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/web/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/webservices/pom.xml (payara-ci) No new issues
Details
security/snyk - copyright/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/admin/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/cluster/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/common/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/core/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/deployment/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/diagnostics/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/distributions/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/flashlight/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/grizzly/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/hk2/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/osgi-platforms/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/packager/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/payara-modules/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/resources-l10n/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/resources/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/security/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/test-utils/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/tests/pom.xml (payara-ci) No new issues
Details
security/snyk - pom.xml (payara-ci) No new issues
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.