[FABG-948] Add support for extra hosts at enrollment #46

vieirin · 2020-02-05T20:37:43Z

Provide WithHosts option and add hosts to CSR info at enrollment request
This allows us to pass extra hosts for certificate enrollment, so it can be valid to
those hosts as well for its CN

Signed-off-by: Manoel [email protected]

Provide WithHosts option and add hosts to CSR info at enrollment request This allows us to pass extra hosts for certificate enrollment, so it can be valid to those hosts as well for its CN Signed-off-by: Manoel <[email protected]>

codecov · 2020-02-05T20:50:04Z

Codecov Report

❗ No coverage uploaded for pull request base (master@070ee52). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master      #46   +/-   ##
=========================================
  Coverage          ?   74.54%           
=========================================
  Files             ?      176           
  Lines             ?    12605           
  Branches          ?        0           
=========================================
  Hits              ?     9396           
  Misses            ?     2289           
  Partials          ?      920

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 070ee52...de91daf. Read the comment docs.

Signed-off-by: Manoel <[email protected]>

alikic

EnrollmentRequest should follow

fabric-sdk-go/internal/github.com/hyperledger/fabric-ca/api/client.go

Line 54 in b5424d1

type EnrollmentRequest struct {

. Adding just host is not a good idea as it is conceivable one might want to add more from CSRInfo in the future. So the whole CSRInfo should be added to EnrollmentRequest instead of just host.

Signed-off-by: Manoel <[email protected]>

vieirin · 2020-02-10T17:11:53Z

@alikic any other changes?

alikic · 2020-02-14T01:33:41Z

pkg/client/msp/ca.go

@@ -6,6 +6,8 @@ SPDX-License-Identifier: Apache-2.0

 package msp

+import "github.com/cloudflare/cfssl/csr"


We don't include third party structures in our high level API. This import shouldn't be here.

alikic · 2020-02-14T01:40:10Z

pkg/client/msp/ca.go

+	Names        []csr.Name
+	Hosts        []string
+	KeyRequest   *BasicKeyRequest
+	CA           *csr.CAConfig


This API shouldn't have dependencies on "github.com/cloudflare/cfssl/csr". CAConfig should be added to our API, and any mapping between this API and third party should be hidden. While we have a freedom to design our API structures independently from the third party, it is useful to keep them close as we keep extending support for Fabric CA features.

I strongly disagree, to create a new struct and re-map it as done in CSR struct seems a bad thing to me, caapi depends on cloudflare structure, this dependency is already packaged with sdk code, btw.
I don't see how creating same structs and "convert this type to that type" functions, where those types have the exact same fields but are different definitions is beneficial

what do you think

Neither fabric-ca nor cloudflare/cfssl are used directly in the exposed API, all uses are internal. I see that we do use cloudflare/cfssl in a couple of tests, perhaps we shouldn't, we can easily change that. Usage of cloudflare/cfssl internally is an implementation detail, shouldn't be in public API.

You wanted to add only hosts to the API call, and my thinking was, if we keep adding the missing pieces of Fabric CA support, we will likely end up with structures similar to what the CA is using. This is why I thought we should introduce CSRInfo, to group hosts and future (currently missing) elements together in a way it makes sense. CSRInfo could initially have only hosts, and whatever you currently need. Ideally, all additions should have proper integration tests.

chanioxaris · 2020-02-26T08:47:17Z

Any updates on this issue?

alikic · 2020-02-26T12:29:46Z

Here is where we stand - the feature is useful, but the implementation has two issues. It introduces a third party dependency in the public API, and it is missing an integration test which would verify that the resulting certificate has desired properties. It makes sense to group all parameters relevant to the creation of a CSR into a structure (perhaps initially having only hosts in the structure), which could be called CSRInfo, but no third party structures are allowed in the public API.

vieirin · 2020-02-28T15:54:58Z

I'm going back to this issue, @alikic this test should be written in integration tests too, right?
On unit tests return for hosts entry is always empty on returned certificate.

Signed-off-by: Manoel <[email protected]>

vieirin · 2020-02-28T17:48:44Z

Integration tests are broken again
updating with last master hope it fix

vieirin · 2020-02-28T18:12:55Z

Done :)

alikic · 2020-02-28T20:02:20Z

test/integration/pkg/client/msp/enrollment_test.go

+		t.Fatalf("Registration failed: %s", err)
+	}
+
+	extraHosts := []string{"localhost"}


Need to test with more values in extraHosts, it should add all of them to the cert....

Can this comparison be O(n²) in test or should it be optimized with a map-based search?
I mean can I compare all requested hosts with all received hosts or may I convert requested hosts to a map first to compare? Does it makes difference? Just to write it right from start haha

Haha, you can implement comparison any way you want, I am just looking for having at least two values there, just to have a little more confidence that the mapping works correctly through the layers all the way to the CA itself.

Ok. Understood.

alikic · 2020-02-28T20:03:15Z

test/integration/pkg/client/msp/enrollment_test.go

+		t.Fatalf("GetSigningIdentity failed: %s", err)
+	}
+
+	has, err := hasHost(si, extraHosts[0])


.... and then verify that all the requested hosts are in the cert.

alikic · 2020-02-28T20:39:38Z

test/integration/pkg/client/msp/enrollment_test.go

+		t.Fatalf("GetSigningIdentity failed: %s", err)
+	}
+
+	has, err = hasHost(si, extraHosts[0])


.... and then verify that all the requested hosts are in the cert.

otherwise looks good

Signed-off-by: Manoel <[email protected]>

[FABG-948] Add support for extra hosts at enrollment

2a9182d

Provide WithHosts option and add hosts to CSR info at enrollment request This allows us to pass extra hosts for certificate enrollment, so it can be valid to those hosts as well for its CN Signed-off-by: Manoel <[email protected]>

vieirin requested a review from a team as a code owner February 5, 2020 20:37

Added test for WithHosts func

d87e6c5

Signed-off-by: Manoel <[email protected]>

vieirin requested a review from bstasyszyn February 6, 2020 17:18

alikic suggested changes Feb 7, 2020

View reviewed changes

vieirin force-pushed the master branch 5 times, most recently from 5eed268 to 91aeb64 Compare February 7, 2020 17:47

Add CSR info to enroll/reenroll request instead of only hosts

9175b27

Signed-off-by: Manoel <[email protected]>

vieirin force-pushed the master branch from 91aeb64 to 9175b27 Compare February 7, 2020 18:12

vieirin requested a review from alikic February 7, 2020 18:25

Merge branch 'master' into master

ac350cb

alikic suggested changes Feb 14, 2020

View reviewed changes

vieirin force-pushed the master branch 2 times, most recently from b15e9ac to aeb3136 Compare February 28, 2020 17:19

Add integration test for enrollment WithCSR

9094f25

Signed-off-by: Manoel <[email protected]>

vieirin force-pushed the master branch from aeb3136 to 9094f25 Compare February 28, 2020 17:32

Merge branch 'master' into master

a6e22fc

alikic suggested changes Feb 28, 2020

View reviewed changes

vieirin added 2 commits March 2, 2020 11:04

Test for all entries on cert hosts

fdf3293

Signed-off-by: Manoel <[email protected]>

Merge branch 'master' of github.com:vieiramanoel/fabric-sdk-go

070ee52

alikic approved these changes Mar 2, 2020

View reviewed changes

Merge branch 'master' into master

de91daf

alikic merged commit ab4de7e into hyperledger:master Mar 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FABG-948] Add support for extra hosts at enrollment #46

[FABG-948] Add support for extra hosts at enrollment #46

vieirin commented Feb 5, 2020

codecov bot commented Feb 5, 2020 •

edited

Loading

alikic left a comment

vieirin commented Feb 10, 2020

alikic Feb 14, 2020

alikic Feb 14, 2020 •

edited

Loading

vieirin Feb 14, 2020 •

edited

Loading

alikic Feb 14, 2020 •

edited

Loading

chanioxaris commented Feb 26, 2020

alikic commented Feb 26, 2020 •

edited

Loading

vieirin commented Feb 28, 2020 •

edited

Loading

vieirin commented Feb 28, 2020

vieirin commented Feb 28, 2020

alikic Feb 28, 2020

vieirin Feb 28, 2020 •

edited

Loading

alikic Feb 28, 2020

vieirin Feb 28, 2020

alikic Feb 28, 2020

alikic Feb 28, 2020

alikic Feb 28, 2020

		@@ -6,6 +6,8 @@ SPDX-License-Identifier: Apache-2.0

		package msp

		import "github.com/cloudflare/cfssl/csr"

[FABG-948] Add support for extra hosts at enrollment #46

[FABG-948] Add support for extra hosts at enrollment #46

Conversation

vieirin commented Feb 5, 2020

codecov bot commented Feb 5, 2020 • edited Loading

Codecov Report

alikic left a comment

Choose a reason for hiding this comment

vieirin commented Feb 10, 2020

alikic Feb 14, 2020

Choose a reason for hiding this comment

alikic Feb 14, 2020 • edited Loading

Choose a reason for hiding this comment

vieirin Feb 14, 2020 • edited Loading

Choose a reason for hiding this comment

alikic Feb 14, 2020 • edited Loading

Choose a reason for hiding this comment

chanioxaris commented Feb 26, 2020

alikic commented Feb 26, 2020 • edited Loading

vieirin commented Feb 28, 2020 • edited Loading

vieirin commented Feb 28, 2020

vieirin commented Feb 28, 2020

alikic Feb 28, 2020

Choose a reason for hiding this comment

vieirin Feb 28, 2020 • edited Loading

Choose a reason for hiding this comment

alikic Feb 28, 2020

Choose a reason for hiding this comment

vieirin Feb 28, 2020

Choose a reason for hiding this comment

alikic Feb 28, 2020

Choose a reason for hiding this comment

alikic Feb 28, 2020

Choose a reason for hiding this comment

alikic Feb 28, 2020

Choose a reason for hiding this comment

codecov bot commented Feb 5, 2020 •

edited

Loading

alikic Feb 14, 2020 •

edited

Loading

vieirin Feb 14, 2020 •

edited

Loading

alikic Feb 14, 2020 •

edited

Loading

alikic commented Feb 26, 2020 •

edited

Loading

vieirin commented Feb 28, 2020 •

edited

Loading

vieirin Feb 28, 2020 •

edited

Loading