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

Make mTLS optional when defining a hook #317

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@aaslamin
Copy link
Contributor

commented May 8, 2019

Issue: aporeto-inc/aporeto#1274

Context: when creating a hook, we want to relax the requirement that mTLS must be configured.

This PR makes the destination endpoint TLS configuration optional and adds a custom validator on the model to validate the optional attributes of clientCertificate & clientCertificateKey. The validator ensures that if either one of these attributes are provided, then BOTH must be provided even though the attributes are individually optional. In the event that a validation error is encountered, an elemental error is thrown which provides the user-agent with context on which attributes were provided and missing.

TODO:

  • Get early feedback from Chris & Antoine to see if I am headed in the right direction
  • Unit tests for custom validator

@aaslamin aaslamin requested review from primalmotion and t00f May 9, 2019

@aaslamin aaslamin force-pushed the project-421 branch from 044624c to 416a0fa May 9, 2019

@aaslamin

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

@t00f @primalmotion Update: I have now added the custom validation here as per your suggestion.

If it looks A-OK to you, I can start writing the unit tests for the validator 🙏

@aaslamin aaslamin changed the title edit: make mTLS fields optional on the 'hookpolicy' model Make mTLS fields optional on the 'hookpolicy' model May 9, 2019

@aaslamin aaslamin changed the title Make mTLS fields optional on the 'hookpolicy' model Make mTLS optional when defining a hook May 9, 2019

@primalmotion
Copy link
Member

left a comment

can you also elaborate the needed values in the attribute description as it becomes less obvious what the user should input

@@ -0,0 +1,55 @@
package gaia

This comment has been minimized.

Copy link
@primalmotion

primalmotion May 9, 2019

Member

put that in custom_validations.go near where it used otherwise you’ll have trouble building. we know we need to find a better way to handle this validations but we have cyclic import issue

This comment has been minimized.

Copy link
@aaslamin

aaslamin May 9, 2019

Author Contributor

done


for _, attr := range attrs {
switch {
case len(attr.value) > 0:

This comment has been minimized.

Copy link
@primalmotion

primalmotion May 9, 2019

Member

for this binary check, please use a if/else. also always check for empty strings using == "". this saves one call, and ensure build breaks if the left operand becomes a map or a slice

This comment has been minimized.

Copy link
@aaslamin

aaslamin May 9, 2019

Author Contributor

and ensure build breaks if the left operand becomes a map or a slice

Nice, didn't even think of that 😉

Donezo

fmt.Sprintf("Attribute(s) [%s] are required because attribute(s) [%s] were provided", strings.Join(missing, ", "), strings.Join(provided, ", ")),
"elemental",
http.StatusUnprocessableEntity)
return err

This comment has been minimized.

Copy link
@primalmotion

primalmotion May 9, 2019

Member

why do you assign it to err? return it directly

This comment has been minimized.

Copy link
@aaslamin

aaslamin May 9, 2019

Author Contributor

Done! 💫

@aaslamin aaslamin force-pushed the project-421 branch from 416a0fa to 7eb44b8 May 9, 2019

@aaslamin

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

Update:

• Moved validation helpers to custom_validations.go near where they are used
• Updated docs to be more clear when clientCertificate & clientCertificateKey need to be provided.

@aaslamin aaslamin requested a review from primalmotion May 9, 2019

@t00f
Copy link
Member

left a comment

One minor comment, otherwise, looks good to me ! 👍

// ValidateClientTLSConfig validates the optional 'clientCertificate' & 'clientCertificateKey' attributes of the hook model.
// The validator ensures that if either one of these attributes are provided, then BOTH should be provided even though the
// attributes are individually optional.
func ValidateClientTLSConfig(hp *HookPolicy) error {

This comment has been minimized.

Copy link
@t00f

t00f May 9, 2019

Member

When reading ValidateClientTLSConfig I expect to pass an TLSConfig object. As I am passing a hookpolicy, we should call it ValidateHookPolicy

This comment has been minimized.

Copy link
@aaslamin

aaslamin May 9, 2019

Author Contributor

Good 👁, updated that now to be more clear! 🙃

@aaslamin aaslamin force-pushed the project-421 branch from dbdff96 to 601fc26 May 9, 2019

@aaslamin

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

Update:

• Renamed ValidateClientTLSConfig ➡️ ValidateHookPolicy

Thanks gents, I will start plumbing some unit tests now for the custom validator.

@aaslamin aaslamin requested a review from t00f May 9, 2019

@aaslamin

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

Update

• Added 100% unit test coverage for the new custom validator - ValidateHookPolicy

I am ready for another review! 🙏

MHcCAQEEIGOXJI/123456789oamOu4tQAIKFdbyvkIJg9GME0mHzoAoGCCqGSM49
AwEHoUQDQgAE6bM8mP123456789AfmBWtnucfByQXk568lDcKNIQx6yNn+7txbwg
F9eXFkofGX3UgRtsHe123456789xQ1naSw==
-----END EC PRIVATE KEY-----`,

This comment has been minimized.

Copy link
@aaslamin

aaslamin May 9, 2019

Author Contributor

Nit: I don't actually need to use realistic values here to test the behaviour I am targeting, just thought it would be nice to make it look real 🤷‍♂

This comment has been minimized.

Copy link
@primalmotion

primalmotion May 10, 2019

Member

Good because soon you will need to :)

}
}

if nonEmptyAttrExists && emptyAttrExists {

This comment has been minimized.

Copy link
@primalmotion

primalmotion May 9, 2019

Member

I just realized you don't need these booleans.

if len(missing) > 0 &&  len(provided) > 0

Always tend to avoid any useless allocation when possible and when it doesn't make the code unreadable.

This comment has been minimized.

Copy link
@aaslamin

aaslamin May 9, 2019

Author Contributor

🦅👁 nice catch!

This comment has been minimized.

Copy link
@aaslamin

aaslamin May 9, 2019

Author Contributor

Donezo

//
// which will return an error because Attr2 was empty and Attr1 was set.
func validateAllOrNoneAttrGrouping(attrs ...attrInfo) error {
var missing, provided []string

This comment has been minimized.

Copy link
@primalmotion

primalmotion May 9, 2019

Member

also skip a line between functions prototype and body when more than 1 or 2 lines

This comment has been minimized.

Copy link
@aaslamin

aaslamin May 9, 2019

Author Contributor

Done

@aaslamin aaslamin requested a review from primalmotion May 9, 2019

aaslamin added some commits May 8, 2019

edit: make mTLS fields optional on hookpolicy model
Signed-off-by: Amir Aslaminejad <amir.a@aporeto.com>
fix: typo in hookpolicy endpoint attribute description
Signed-off-by: Amir Aslaminejad <amir.a@aporeto.com>
new: add validation helper for validating a grouping of related attri…
…butes - useful for when you need to check if a group of optional attributes are either all provided or not at all

new: add custom validation on hookpolicies model for the validation of the now optional client mTLS attributes - clientCertificate & clientCertificateKey - if either one is provided, they now must BOTH be provided

Signed-off-by: Amir Aslaminejad <amir.a@aporeto.com>
docs: update description on clientCertificate & clientCertificateKey …
…attributes to when they should be provided

Signed-off-by: Amir Aslaminejad <amir.a@aporeto.com>
fix: remove unnecessary allocation of booleans from validation helper
Signed-off-by: Amir Aslaminejad <amir.a@aporeto.com>

@aaslamin aaslamin force-pushed the project-421 branch from 669fb60 to 9d11b15 May 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.