Skip to content
This repository has been archived by the owner on Apr 11, 2023. It is now read-only.

Add {Peer,Request}Authentication objects to Create Istio Config #1804

Merged
merged 12 commits into from May 26, 2020

Conversation

lucasponce
Copy link
Contributor

@lucasponce lucasponce commented May 21, 2020

This PR adds support to PeerAuthentication and RequestAuthentication objects in Create New IstioConfig.

Now together with AuthorizationPolicy, Kiali should be able to create all type of Security scenarios.

Requires kiali/kiali#2797

How to test:

Fixes kiali/kiali#2797
Fixes kiali/kiali#2685
Fixes kiali/kiali#2694

@lucasponce lucasponce added the requires server PR A PR sent to the frontend kiali/kiali-ui requires changes on backend kiali/kiali label May 21, 2020
@lucasponce lucasponce added this to In Review in Sprint 40 May 21, 2020
@lucasponce lucasponce requested a review from skondkar May 21, 2020 11:33
Copy link
Contributor

@hhovsepy hhovsepy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lucasponce wdyt, from UX perspective, is't it better to disable Create button when Add Workload Selector and Add Port MTLS are checked but no values are given?
Screenshot from 2020-05-21 15-18-42

@lucasponce
Copy link
Contributor Author

@lucasponce wdyt, from UX perspective, is't it better to disable Create button when Add Workload Selector and Add Port MTLS are checked but no values are given?

+1

@hhovsepy
Copy link
Contributor

hhovsepy commented May 21, 2020

Please add regex check on RequestAuthentication name to disable the Create button if the name is wrong.

@xeviknal
Copy link
Member

Creating a PeerAuthn, the Mutual TLS mode is always set to 'UNSET'. However, portLevelTLS mode works correctly.

peerauthn-unset

@xeviknal
Copy link
Member

I see in the RequestAuthentication that the fromHeaders field is an string input text whereas in the documentation says it is an object.

Screenshot of Istio _ JWTRule (1)

Screenshot of Kiali Console (81)

Is it intended to be like this? Perhaps implemented in a subsequent phase? If so, shouldn't we remove this option from the list?

@lucasponce
Copy link
Contributor Author

@hhovsepy yes, you are right, I'm going to incorporate these validations, also extend them to
kiali/kiali#2694

@lucasponce
Copy link
Contributor Author

lucasponce commented May 22, 2020

@xeviknal

Is it intended to be like this? Perhaps implemented in a subsequent phase? If so, shouldn't we remove this option from the list?

Mmm in the form it's showed as "Header: value, Header2: value2", but when you create the yaml should be created as an object.

It's just visualization in the form is simpler just to match the pattern used by curl or similar.

I'll add helper text describing the format, and a validation there.

@lucasponce
Copy link
Contributor Author

@hhovsepy @xeviknal I have addressed most of the validations and comments.

Basically I refactored the IstioConfigNewPage to have a common pattern.
This started with Sidecar and Gateways and now with more objects it was necessary to have some changes to proper add validations at different levels (common, like name, namespace) and specific of each object.

There is a pending work I'm reviewing, as Gateways have changed in Istio 1.6 and scenarios with https doesn't work well in the IstioConfig form and wizards, but I will probably send that in a separate PR to not increase this one without context.

Please, let me know if it works ok, @hhovsepy @xeviknal take care of all types, probably I may introduce some regression as the changes were important.

Thanks

@lucasponce lucasponce force-pushed the peer-and-request-authentication branch from 660e28c to 072df14 Compare May 25, 2020 12:24
@lucasponce lucasponce force-pushed the peer-and-request-authentication branch from 072df14 to b0bd305 Compare May 25, 2020 12:30
@hhovsepy
Copy link
Contributor

Tried all JWT rules :)
"forwardOriginalToken"'s value is ignored to add.
When all rule types are added, the empty dropdown with rule type is remaining, not critical but from UX perspective better to hide that line at all:
Screenshot from 2020-05-25 15-53-25

@hhovsepy hhovsepy self-requested a review May 25, 2020 13:57
@lucasponce
Copy link
Contributor Author

When all rule types are added, the empty dropdown with rule type is remaining, not critical but from UX perspective better to hide that line at all:

+1

It happens to other builders that use the same technique to remove available fields from a select.
So perhaps that change may apply to other places in AuthorizationPolicy as well.

@xeviknal
Copy link
Member

I see that the jwks field disappears once you click to the '+' button.
I don't see that field either on @hhovsepy screenshot.

peerauthn-jwks

@lucasponce
Copy link
Contributor Author

@xeviknal @hhovsepy I've addressed your comments, let me know how it looks.

@hhovsepy
Copy link
Contributor

I can still see 2 problems:

  1. "forwardOriginalToken"'s value is ignored to add.
  2. "jwksUri" value causes error while creating. In logs: "admission webhook "validation.istio.io" denied the request: configuration is invalid: 1 error occurred:
    • URI scheme "" is not supported"

@xeviknal
Copy link
Member

I am seeing this:

peerauthn-jwks

@lucasponce
Copy link
Contributor Author

@hhovsepy @xeviknal you are right, I added the jwks into the JWT Rule Builder but there was a missing edge in the JWT List.

Also added uri validation for builder to only delegate on galley in more corner case scenarios.

Copy link
Member

@xeviknal xeviknal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small comment added. LGTM besides that :)

@hhovsepy
Copy link
Contributor

All my issues are fixed, thank @lucasponce .
Only small comment, when adding first JWT Rule, for the second one in the input it selects by default "audiences" rule and shows warning that "issuer" is required.
Screenshot from 2020-05-26 10-38-39

@lucasponce
Copy link
Contributor Author

Only small comment, when adding first JWT Rule, for the second one in the input it selects by default "audiences" rule and shows warning that "issuer" is required.

That is expected, JWT Rule Builder can create 1 to many JWT Rules. Each JWT Rule should have an issuer. Any row in the JWT Rules List is a JWT Rule that must have an issuer field.

Copy link
Contributor

@hhovsepy hhovsepy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified.

@hhovsepy
Copy link
Contributor

Verified also fix of: kiali/kiali#2694

@lucasponce lucasponce merged commit e4b2f24 into kiali:master May 26, 2020
Sprint 40 automation moved this from In Review to Done May 26, 2020
@ghost ghost added this to the v1.19.0 milestone May 26, 2020
lucasponce added a commit to lucasponce/kiali-ui that referenced this pull request May 26, 2020
…i#1804)

* Add {Peer,Request}Authentication objects to Create Istio Config

* Fix correct iteration method

* Fix PeerAuthentication state

* Refactor IstioConfigNewPage for better place of validations

* Fix linter

* PeerAuthentication validations

* Add RequestAuthentication validation

* Prettier fixes

* Fix ci errors

* Address Hayk and Xavi's comments

* Fix jwks and formatOriginalToken issues

* Fix debug console logs
lucasponce added a commit that referenced this pull request May 26, 2020
…ig (#1807)

* Add {Peer,Request}Authentication objects to Create Istio Config (#1804)

* Add {Peer,Request}Authentication objects to Create Istio Config

* Fix correct iteration method

* Fix PeerAuthentication state

* Refactor IstioConfigNewPage for better place of validations

* Fix linter

* PeerAuthentication validations

* Add RequestAuthentication validation

* Prettier fixes

* Fix ci errors

* Address Hayk and Xavi's comments

* Fix jwks and formatOriginalToken issues

* Fix debug console logs

* Adjust prettier to v1.18 style
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
requires server PR A PR sent to the frontend kiali/kiali-ui requires changes on backend kiali/kiali
Projects
No open projects
Sprint 40
  
Done
3 participants