Add more validations to trust-policy parsing #43

rgnote · 2022-05-12T01:33:32Z

Changes:

Added Registry Scope validations using the regex from distribution spec
Added DN validations using go-ldap package
Moved the policy document validation logic to sub-functions for better readability
Moved the verification utility functions to helpers.go

Testing:
Added more unit tests and extensively tested the logic with local dummy policy files

Signed-off-by: rgnote 5878554+rgnote@users.noreply.github.com

Signed-off-by: rgnote <5878554+rgnote@users.noreply.github.com>

qmuntal · 2022-05-12T18:31:07Z

go.mod

@@ -6,3 +6,8 @@ require (
 	github.com/golang-jwt/jwt/v4 v4.4.1
 	github.com/opencontainers/go-digest v1.0.0
 )
+
+require (
+	github.com/go-ldap/ldap v3.0.3+incompatible // indirect


How is that ldap is marked as indirect when it is directly referenced in our code?

I should have run go mod tidy. Also, I realized that I should use go-ldap v3 as mentioned here. Thanks for pointing this out. Will raise a new revision for this PR.

Signed-off-by: rgnote <5878554+rgnote@users.noreply.github.com>

gokarnm · 2022-05-13T16:37:06Z

go.mod

@@ -3,6 +3,13 @@ module github.com/notaryproject/notation-go
 go 1.17

 require (
+	github.com/go-ldap/ldap/v3 v3.4.3


go-ldap has a dependency on github.com/Azure/go-ntlmssp, but the functionality we need, DN validation does not. Is there a way to avoid this transitive dependency? Is the transitive dependency dropped during build as we do not use any types from github.com/Azure/go-ntlmssp?

AFAIK, final build will only include the specific methods and their dependencies. Although I could not confirm this with official Go docs, I found multiple online forums confirming this. Seems like https://github.com/jondot/goweight could help us see what's included in the final build. I added it to my TODO list to revisit later.

unpopular opinion: Pulling in github.com/go-ldap/ldap, which is a library to talk with LDAP servers, just for validating a DN, seems like using a sledgehammer to crack a nut.

I would argue it's better just to copy ldap.ParseDN, and related tests, into notation-go and remove that dependency.

@qmuntal If we consume the whole package in our builds (like in java), then I agree with what you said. Thanks to Go static linking, our build will only include dn.ParseDN and it's dependencies. I prefer to keep a dependency on go-ldap to ensure we get the updates and security fixes. Also, go-ldap is the most popular library for ldap in Go, I'm not too worried about having a dependency on it.

Besides, I want to give them the credit rather than simply copying their code :)

I'm not that concerned about binary size, as Go is really good at prunning unnecessary code. I'm worried about having a sane dependency tree. Keeping it to the bare minimum is always a plus, even if it means duplicating code.

That said, this particular dependency is not a red line for me.

One concern I had was, we would see patch updates to this package more frequently from go-ldap/ldap package than we'd be actually impacted, as we are using a single type from the whole package. What is generally the process to handle patch updates to dependencies, will dependabot make this simpler?

@rgnote it's possible to copy a code files into our repo and still provide correct attribution to source repo.

I'd prefer copying the code in this case, it's not a one way decision, we can change it the other way if we wanted to later.

Tracking this in #45

gokarnm · 2022-05-13T16:43:04Z

verification/helpers.go

+}
+
+// validateDistinguishedName validates if a DN name parsable and follows Notary V2 rules
+func validateDistinguishedName(name string) error {


IMO, this method should move into notation-core-go. All X509 related parsing, validation, cert chain validation logic should be pushed there.

On second thought, this is Notary V2 specific DN validation based on what we allow in trust policy, so seems appropriate here.

Should this method and validateRegistryScopeFormat be part of policy.go, they are only used in the context of validating a trust policy statement.

These helpers are for policy validation but I wanted to move them to a helper file so that policy.go is cleaner and deals with higher level functionality like policy verification. There is no side effect of having these methods in a helper class as Go package is a closure of everything under the package directory.

gokarnm · 2022-05-13T16:54:28Z

verification/helpers.go

+	dn, err := ldapv3.ParseDN(name)
+
+	if err != nil {
+		return fmt.Errorf("distinguished name (DN) %q is not valid, make sure it is following rfc4514 standard", name)


Suggested change

return fmt.Errorf("distinguished name (DN) %q is not valid, make sure it is following rfc4514 standard", name)

return fmt.Errorf("distinguished name (DN) %q is not valid, it must contain 'C', 'ST', and 'O' RDN attributes at a minimum, and follow RFC 4514 standard", name)

gokarnm · 2022-05-13T17:47:02Z

verification/helpers.go

+	// Verify there are no duplicate RDNs (multi-valdued RDNs are not supported)
+	for key := range rDnCount {
+		if rDnCount[key] > 1 {
+			return fmt.Errorf("distinguished name (DN) %q has duplicate RDNs, DN can only have unique RDNs", name)
+		}
+	}


Duplicate is different from multi valued RDN, correct?
Multi-valued - OU=Sales+CN=J. Smith,DC=example
Duplicate -OU=Sales,DC=example,DC=net

End customers may not be familiar with terminology RDN

Suggested change

// Verify there are no duplicate RDNs (multi-valdued RDNs are not supported)

for key := range rDnCount {

if rDnCount[key] > 1 {

return fmt.Errorf("distinguished name (DN) %q has duplicate RDNs, DN can only have unique RDNs", name)

}

}

// Verify there are no duplicate RDNs (multi-valdued RDNs are not supported)

for key := range rDnCount {

if rDnCount[key] > 1 {

return fmt.Errorf("distinguished name (DN) %q has duplicate RDN attribute for %q, DN can only have unique RDN attributes", name, key )

}

}

go-ldap does support multi-valued RDNs, hence notation-go lib can also support them. However, we should reject duplicate RDNs (OU=Sales,DC=example,DC=net) as there is no usecase for them in notary V2.

We want notation-go lib to not support multi-valued RDNs as they are not commonly used, and confusing for end customers.

gokarnm · 2022-05-13T17:54:48Z

verification/helpers.go

+	// Verify mandatory fields are present
+	for _, field := range mandatoryFields {
+		if rDnCount[field] != 1 {
+			return fmt.Errorf("distinguished name (DN) %q has no mandatory RDN for %q", name, field)


Suggested change

return fmt.Errorf("distinguished name (DN) %q has no mandatory RDN for %q", name, field)

return fmt.Errorf("distinguished name (DN) %q has no mandatory RDN attribute for %q, it must contain 'C', 'ST', and 'O' RDN attributes at a minimum", name, field)

gokarnm · 2022-05-17T16:56:31Z

go.mod

@@ -3,6 +3,13 @@ module github.com/notaryproject/notation-go
 go 1.17

 require (
+	github.com/go-ldap/ldap/v3 v3.4.3


One concern I had was, we would see patch updates to this package more frequently from go-ldap/ldap package than we'd be actually impacted, as we are using a single type from the whole package. What is generally the process to handle patch updates to dependencies, will dependabot make this simpler?

@rgnote it's possible to copy a code files into our repo and still provide correct attribution to source repo.

I'd prefer copying the code in this case, it's not a one way decision, we can change it the other way if we wanted to later.

Signed-off-by: rgnote <5878554+rgnote@users.noreply.github.com>

rgnote · 2022-05-19T21:31:21Z

@qmuntal @gokarnm Addressed both your comments

qmuntal

LGTM!

Signed-off-by: rgnote <5878554+rgnote@users.noreply.github.com>

rgnote · 2022-05-24T18:12:48Z

@gokarnm @qmuntal
Included two more validations in the latest version

Throw error if there are any overlapping DNs in trustedIdentities
Throw error if there are multi-valued RDNs in a DN

NiazFK

LGTM

Add more validations to trust-policy parsing

0db6351

Signed-off-by: rgnote <5878554+rgnote@users.noreply.github.com>

rgnote requested a review from a team May 12, 2022 01:33

qmuntal reviewed May 12, 2022

View reviewed changes

use go-ldap/v3

1546c20

Signed-off-by: rgnote <5878554+rgnote@users.noreply.github.com>

gokarnm reviewed May 13, 2022

View reviewed changes

gokarnm reviewed May 17, 2022

View reviewed changes

more descriptive error messages

3802596

Signed-off-by: rgnote <5878554+rgnote@users.noreply.github.com>

qmuntal approved these changes May 19, 2022

View reviewed changes

validations for overlapping DNs and multi-valued RDNs

dadad9f

Signed-off-by: rgnote <5878554+rgnote@users.noreply.github.com>

gokarnm approved these changes May 26, 2022

View reviewed changes

NiazFK approved these changes May 26, 2022

View reviewed changes

NiazFK merged commit 2c9a8b0 into notaryproject:main May 26, 2022

iamsamirzon added this to the RC-1 milestone Jul 14, 2022

iamsamirzon assigned rgnote Jul 14, 2022

dtzar modified the milestones: RC-1, alpha-3 Jul 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more validations to trust-policy parsing #43

Add more validations to trust-policy parsing #43

rgnote commented May 12, 2022

qmuntal May 12, 2022

rgnote May 12, 2022

gokarnm May 13, 2022

rgnote May 13, 2022

qmuntal May 14, 2022

rgnote May 16, 2022

rgnote May 16, 2022

qmuntal May 17, 2022 •

edited

Loading

gokarnm May 17, 2022

rgnote May 19, 2022

gokarnm May 13, 2022 •

edited

Loading

gokarnm May 13, 2022

rgnote May 13, 2022

gokarnm May 13, 2022

gokarnm May 13, 2022

gokarnm May 13, 2022

rgnote May 19, 2022

gokarnm May 20, 2022

rgnote May 24, 2022

gokarnm May 13, 2022

gokarnm May 17, 2022

rgnote commented May 19, 2022

qmuntal left a comment

rgnote commented May 24, 2022

NiazFK left a comment

	return fmt.Errorf("distinguished name (DN) %q is not valid, make sure it is following rfc4514 standard", name)
	return fmt.Errorf("distinguished name (DN) %q is not valid, it must contain 'C', 'ST', and 'O' RDN attributes at a minimum, and follow RFC 4514 standard", name)

Add more validations to trust-policy parsing #43

Add more validations to trust-policy parsing #43

Conversation

rgnote commented May 12, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qmuntal May 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gokarnm May 13, 2022 • edited Loading

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

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

Choose a reason for hiding this comment

rgnote commented May 19, 2022

qmuntal left a comment

Choose a reason for hiding this comment

rgnote commented May 24, 2022

NiazFK left a comment

Choose a reason for hiding this comment

qmuntal May 17, 2022 •

edited

Loading

gokarnm May 13, 2022 •

edited

Loading