Support Out-Of-the-Box mTLS auth for interoperator broker. #1464

Pooja-08 · 2021-11-08T06:21:55Z

No description provided.

cf-gitbot · 2021-11-08T06:21:56Z

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/180226462

The labels on this github issue will be updated when the story is started.

codecov · 2021-11-08T06:27:53Z

Codecov Report

Merging #1464 (0185814) into master (b21442c) will decrease coverage by 0.23%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1464      +/-   ##
==========================================
- Coverage   73.07%   72.84%   -0.24%     
==========================================
  Files          45       45              
  Lines        4290     4290              
==========================================
- Hits         3135     3125      -10     
- Misses        825      832       +7     
- Partials      330      333       +3

vinaybheri · 2021-11-10T06:53:29Z

Validation on k8s cluster: n failed.

vinaybheri · 2021-11-10T06:53:42Z

Validation on k8s cluster: n-2 failed.

vinaybheri · 2021-11-10T06:53:48Z

Validation on k8s cluster: n-1 failed.

vinaybheri · 2021-11-10T06:53:55Z

Validation on k8s cluster: n+n+n failed.

coveralls · 2021-11-18T05:10:33Z

Pull Request Test Coverage Report for Build 5709

6 of 26 (23.08%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.2%) to 88.478%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
broker/applications/osb-broker/src/api-controllers/routes/broker/v2.js	3	5	60.0%
broker/core/express-commons/src/middleware/index.js	3	21	14.29%

Totals
Change from base Build 5703:	-0.2%
Covered Lines:	9724
Relevant Lines:	10606

💛 - Coveralls

saud89 · 2021-11-18T06:55:21Z

broker/core/express-commons/src/middleware/index.js

+        }
+        next();
+      } else {
+        return next(new Unauthorized());


Hi @Pooja-08 , a logger before return statement with message stating expected subject pattern not defined would be good to have. Since this is a config parameter, there are high changes teams will forget to add and we might get generic unAuthorized error.

saud89 · 2021-11-18T06:57:13Z

docs/Interoperator-authentication.md

+  - mTLS Authentication: validates the client certificate's subject pattern.
+
+## Basic Authentication
+To use Basic Authentication, the username and password should be provided during the registration of the broker in the Service Manager.


Hi @Pooja-08, This document is specific to SAP and SAP's service manager... might be you can move service manager related mentions to internal github page?

anoopjb · 2021-11-18T10:40:20Z

broker/core/express-commons/src/middleware/index.js

+    const clientCertificate = req.headers['ssl-client-cert'];
+    if (!clientCertificate) {
+      logger.error('clientCertificate not found in the request headers: ', req.headers);
+      next(new Unauthorized());


Suggested change

next(new Unauthorized());

next(new Unauthorized('Client certificate not found in the request headers'));

I feel it will be more informative to the user if we pass the cause of the error. This message will be part of the response. The default message is 'The request requires user authentication'.

anoopjb · 2021-11-18T10:43:59Z

broker/core/express-commons/src/middleware/index.js

+        let subjectDN = req.headers['ssl-client-subject-dn'];
+        if (!subjectDN || !verifySubjectDN(subjectDN, smCertSubjectPattern)) {
+          logger.error('subject DN doesnt match the sm_certificate_subject_pattern, subjectDN: ', subjectDN, ' sm_certificate_subject_pattern: ', smCertSubjectPattern);
+          return next(new Unauthorized());


Suggested change

return next(new Unauthorized());

return next(new Unauthorized('Subject DN in the request header doesn't match the configured sm_certificate_subject_pattern'));

anoopjb · 2021-11-18T10:48:27Z

broker/core/express-commons/src/middleware/index.js

+      logger.error('clientCertificate not found in the request headers: ', req.headers);
+      next(new Unauthorized());
+    } else {
+      const smCertSubjectPattern = config.smConnectionSettings.sm_certificate_subject_pattern;


Suggested change

const smCertSubjectPattern = config.smConnectionSettings.sm_certificate_subject_pattern;

const smCertSubjectPattern = _.get(config, 'smConnectionSettings.sm_certificate_subject_pattern');

I think there may be chances that smConnectionSettings might be commented out in the settings.yml which may lead to panic. lodash.get takes care of this.

Pooja-08 · 2021-11-18T12:49:36Z

@saud89 @anoopjb thanks for providing the comments. Addressed. Please review.
Also, opened a PR in the interoperator-deployment-component repo for the doc. Please review.

https://github.wdf.sap.corp/servicefabrik/interoperator-deployment-component/pull/44

anoopjb · 2021-11-18T15:12:01Z

docs/Interoperator-authentication.md

+
+## Basic Authentication
+To use Basic Authentication, the username and password should be provided during the registration of the broker.
+This is relevant for both subaccount-scoped registration done by smctl, and global registration done by the Cross Registration Service (xRS) API.


Suggested change

This is relevant for both subaccount-scoped registration done by smctl, and global registration done by the Cross Registration Service (xRS) API.

The subaccount-scoped registration and the Cross Registration Service (xRS) API is specific to SAP. I feel we can skip this sentence.

anoopjb · 2021-11-18T15:16:12Z

docs/Interoperator-authentication.md

+## Out-Of-The-Box mTLS Authentication
+Interoperator broker currently supports the Out-Of-The-Box mTLS authentication.


Suggested change

## Out-Of-The-Box mTLS Authentication

Interoperator broker currently supports the Out-Of-The-Box mTLS authentication.

## mTLS Authentication

Interoperator broker currently supports mTLS authentication using Nginx ingress annotations.

I think the "Out-Of-The-Box" catch word is from SAP Service Manager 😄 I could not find documents with Out-Of-The-Box mTLS when I did a quick search. This may lead to confusion.

anoopjb · 2021-11-18T15:20:27Z

docs/Interoperator-authentication.md

+The "sm_certificate_subject_pattern" value must be set to the client certificate's subject, for the authentication to succeed.
+The subject header in the client certificate must contain comma(',') separated values, while the sm_certificate_subject_pattern must contain '/' separated values.
+
+Ref: https://wiki.wdf.sap.corp/wiki/display/CPC15N/Develop#Develop-2.a.ProvideYourOwnmTLS-BasedCertificate


Suggested change

Ref: https://wiki.wdf.sap.corp/wiki/display/CPC15N/Develop#Develop-2.a.ProvideYourOwnmTLS-BasedCertificate

We may need to remove this internal wiki link.

anoopjb · 2021-11-18T15:40:50Z

docs/Interoperator-authentication.md

+  - nginx.ingress.kubernetes.io/auth-tls-verify-client: "on"
+    Must be set to "on" to enable verification of client certificates. When this annotation is set to "on", it requests a client certificate that must be signed by a certificate that is included in the secret key ca.crt of the secret specified by "nginx.ingress.kubernetes.io/auth-tls-secret"
+  - nginx.ingress.kubernetes.io/auth-tls-secret: namespace/secretName
+    This secret must have a file named ca.crt containing the full Certificate Authority chain ca.crt that is enabled to authenticate against this Ingress.
+  - nginx.ingress.kubernetes.io/auth-tls-pass-certificate-to-upstream: true
+    when set to true, passes the received certificates to the upstream server in the header ssl-client-cert. For mTLS auth, this annotation must be set to true.


Suggested change

- nginx.ingress.kubernetes.io/auth-tls-verify-client: "on"

Must be set to "on" to enable verification of client certificates. When this annotation is set to "on", it requests a client certificate that must be signed by a certificate that is included in the secret key ca.crt of the secret specified by "nginx.ingress.kubernetes.io/auth-tls-secret"

- nginx.ingress.kubernetes.io/auth-tls-secret: namespace/secretName

This secret must have a file named ca.crt containing the full Certificate Authority chain ca.crt that is enabled to authenticate against this Ingress.

- nginx.ingress.kubernetes.io/auth-tls-pass-certificate-to-upstream: true

when set to true, passes the received certificates to the upstream server in the header ssl-client-cert. For mTLS auth, this annotation must be set to true.

- `nginx.ingress.kubernetes.io/auth-tls-verify-client: "on"` <br>

Must be set to "on" to enable verification of client certificates. When this annotation is set to "on", it requests a client certificate that must be signed by a certificate that is included in the secret key ca.crt of the secret specified by "nginx.ingress.kubernetes.io/auth-tls-secret"

- `nginx.ingress.kubernetes.io/auth-tls-secret: namespace/secretName` <br>

This secret must have a file named ca.crt containing the full Certificate Authority chain ca.crt that is enabled to authenticate against this Ingress.

- `nginx.ingress.kubernetes.io/auth-tls-pass-certificate-to-upstream: true` <br>

when set to true, passes the received certificates to the upstream server in the header ssl-client-cert. For mTLS auth, this annotation must be set to true.

Minor formatting suggestion. 😸
<br> helps to print its following sentence to a new line.

anoopjb

LGTM 🚢
I added some minor suggestions.

anoopjb · 2021-11-18T15:45:23Z

docs/Interoperator-SM-authentication.md

@@ -0,0 +1,22 @@
+# Service Fabrik Inter-operator Authentication


This looks like a redundant document since the content is similar to docs/Interoperator-authentication.md

…dry#1464) * Add support for mTLS auth in broker * Add documentation for authentication.

cf-gitbot added the unscheduled label Nov 8, 2021

Pooja-08 force-pushed the oob_mtls_support branch from cf0bc61 to 62ccb35 Compare November 8, 2021 06:27

Pooja-08 requested review from anoopjb, saud89 and swati1102 November 10, 2021 06:17

Pooja-08 force-pushed the oob_mtls_support branch from 62ccb35 to 0a99ee0 Compare November 10, 2021 06:39

anoopjb assigned Pooja-08 Nov 17, 2021

Pooja-08 force-pushed the oob_mtls_support branch from 0a99ee0 to a07dec9 Compare November 18, 2021 05:03

saud89 reviewed Nov 18, 2021

View reviewed changes

anoopjb reviewed Nov 18, 2021

View reviewed changes

Pooja-08 force-pushed the oob_mtls_support branch 2 times, most recently from f9f0d57 to 7680dfa Compare November 18, 2021 12:45

anoopjb approved these changes Nov 18, 2021

View reviewed changes

anoopjb reviewed Nov 18, 2021

View reviewed changes

anoopjb approved these changes Nov 18, 2021

View reviewed changes

anoopjb reviewed Nov 18, 2021

View reviewed changes

Add support for mTLS auth in broker

2866dee

Pooja-08 force-pushed the oob_mtls_support branch from 7680dfa to a8b86e1 Compare November 19, 2021 03:03

Add documentation for authentication.

0185814

Pooja-08 force-pushed the oob_mtls_support branch from a8b86e1 to 0185814 Compare November 19, 2021 03:10

saud89 approved these changes Nov 19, 2021

View reviewed changes

Pooja-08 merged commit 256c503 into cloudfoundry:master Nov 19, 2021

cf-gitbot removed the unscheduled label Nov 19, 2021

jintusebastian pushed a commit to jintusebastian/service-fabrik-broker that referenced this pull request May 17, 2022

Support Out-Of-the-Box mTLS auth for interoperator broker. (cloudfoun…

9b37798

…dry#1464) * Add support for mTLS auth in broker * Add documentation for authentication.

jintusebastian pushed a commit to jintusebastian/service-fabrik-broker that referenced this pull request Feb 13, 2023

Support Out-Of-the-Box mTLS auth for interoperator broker. (cloudfoun…

13aa1a1

…dry#1464) * Add support for mTLS auth in broker * Add documentation for authentication.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Out-Of-the-Box mTLS auth for interoperator broker. #1464

Support Out-Of-the-Box mTLS auth for interoperator broker. #1464

Pooja-08 commented Nov 8, 2021

cf-gitbot commented Nov 8, 2021

codecov bot commented Nov 8, 2021 •

edited

Loading

vinaybheri commented Nov 10, 2021

vinaybheri commented Nov 10, 2021

vinaybheri commented Nov 10, 2021

vinaybheri commented Nov 10, 2021

coveralls commented Nov 18, 2021 •

edited

Loading

saud89 Nov 18, 2021

saud89 Nov 18, 2021

anoopjb Nov 18, 2021

anoopjb Nov 18, 2021

anoopjb Nov 18, 2021

Pooja-08 commented Nov 18, 2021

anoopjb Nov 18, 2021

anoopjb Nov 18, 2021 •

edited

Loading

anoopjb Nov 18, 2021

anoopjb Nov 18, 2021 •

edited

Loading

anoopjb left a comment

anoopjb Nov 18, 2021

	next(new Unauthorized());
	next(new Unauthorized('Client certificate not found in the request headers'));

	return next(new Unauthorized());
	return next(new Unauthorized('Subject DN in the request header doesn't match the configured sm_certificate_subject_pattern'));

	const smCertSubjectPattern = config.smConnectionSettings.sm_certificate_subject_pattern;
	const smCertSubjectPattern = _.get(config, 'smConnectionSettings.sm_certificate_subject_pattern');

		## Out-Of-The-Box mTLS Authentication
		Interoperator broker currently supports the Out-Of-The-Box mTLS authentication.

		@@ -0,0 +1,22 @@
		# Service Fabrik Inter-operator Authentication

Support Out-Of-the-Box mTLS auth for interoperator broker. #1464

Support Out-Of-the-Box mTLS auth for interoperator broker. #1464

Conversation

Pooja-08 commented Nov 8, 2021

cf-gitbot commented Nov 8, 2021

codecov bot commented Nov 8, 2021 • edited Loading

Codecov Report

vinaybheri commented Nov 10, 2021

vinaybheri commented Nov 10, 2021

vinaybheri commented Nov 10, 2021

vinaybheri commented Nov 10, 2021

coveralls commented Nov 18, 2021 • edited Loading

Pull Request Test Coverage Report for Build 5709

💛 - Coveralls

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

Pooja-08 commented Nov 18, 2021

Choose a reason for hiding this comment

anoopjb Nov 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anoopjb Nov 18, 2021 • edited Loading

Choose a reason for hiding this comment

anoopjb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 8, 2021 •

edited

Loading

coveralls commented Nov 18, 2021 •

edited

Loading

anoopjb Nov 18, 2021 •

edited

Loading

anoopjb Nov 18, 2021 •

edited

Loading