Skip to content

Adds JWT Authn Support to XDS Translator#934

Merged
danehans merged 1 commit into
envoyproxy:mainfrom
danehans:authen_xds_trans
Feb 1, 2023
Merged

Adds JWT Authn Support to XDS Translator#934
danehans merged 1 commit into
envoyproxy:mainfrom
danehans:authen_xds_trans

Conversation

@danehans
Copy link
Copy Markdown
Contributor

Adds initial JWT authentication support to xDS translator.

Signed-off-by: danehans daneyonhansen@gmail.com

@danehans danehans requested a review from a team as a code owner January 21, 2023 02:28
@danehans danehans added the area/translator Issues related to Gateway's translation service, e.g. translating Gateway APIs into the IR. label Jan 21, 2023
@danehans danehans added this to the 0.3.0-rc.1 milestone Jan 21, 2023
@danehans danehans marked this pull request as draft January 21, 2023 02:28
@danehans
Copy link
Copy Markdown
Contributor Author

Marked PR as a draft until:

  • I can verify the typedPerFilterConfig is constructed properly.
  • Add additional integration tests.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jan 21, 2023

Codecov Report

Merging #934 (5c8705b) into main (84044da) will decrease coverage by 0.02%.
The diff coverage is 66.58%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main     #934      +/-   ##
==========================================
- Coverage   63.84%   63.83%   -0.02%     
==========================================
  Files          54       55       +1     
  Lines        7834     8221     +387     
==========================================
+ Hits         5002     5248     +246     
- Misses       2519     2629     +110     
- Partials      313      344      +31     
Impacted Files Coverage Δ
internal/ir/zz_generated.deepcopy.go 11.66% <0.00%> (-0.69%) ⬇️
internal/xds/translator/listener.go 80.83% <0.00%> (-0.74%) ⬇️
internal/xds/translator/route.go 82.60% <0.00%> (-1.00%) ⬇️
internal/ir/xds.go 75.77% <50.00%> (-0.09%) ⬇️
internal/xds/translator/translator.go 76.04% <64.70%> (+0.55%) ⬆️
internal/xds/translator/ratelimit.go 94.97% <66.66%> (-1.15%) ⬇️
internal/xds/translator/authentication.go 70.03% <70.03%> (ø)
internal/gatewayapi/filters.go 80.84% <96.29%> (+0.36%) ⬆️
api/v1alpha1/validation/authenticationfilter.go 85.24% <100.00%> (+5.70%) ⬆️
internal/gatewayapi/route.go 89.17% <100.00%> (ø)
... and 7 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Per #813 (comment), this PR uses per filter config to configure the JWT provider for matched traffic. @lizan @arkodg PTAL when you have a moment, PTAL and let me know if this looks correct to you. I did not include any match in requirements since the route provides the match.

Copy link
Copy Markdown
Contributor

@arkodg arkodg Jan 24, 2023

Choose a reason for hiding this comment

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

yeah look good from a config perspective

@danehans
Copy link
Copy Markdown
Contributor Author

Removing "draft" since commit a933248 expands xds translator integration tests and the jwt requirements align with upstream docs.

@danehans danehans marked this pull request as ready for review January 24, 2023 02:38
@danehans danehans marked this pull request as draft January 24, 2023 22:03
@danehans
Copy link
Copy Markdown
Contributor Author

Converted back to draft since Envoy is not accepting the jwt config:

cache/logrwrapper.go:29	handling v3 xDS resource request, response_nonce 1, nodeID envoy-default-eg-64656661-7db8b55db6-r25fn, node_version v1.26.0, resource_names_subscribe [], resource_names_unsubscribe [], type_url type.googleapis.com/envoy.config.route.v3.RouteConfiguration, errorCode 13, errorMessage Unable to unpack as envoy.extensions.filters.http.jwt_authn.v3.PerRouteConfig: [type.googleapis.com/envoy.extensions.filters.http.jwt_authn.v3.JwtAuthentication] {
  providers {
    key: "example"
    value {
      issuer: "testing@secure.istio.io"
      remote_jwks {
        http_uri {
          uri: "https://raw.githubusercontent.com/istio/istio/release-1.16/security/tools/jwt/samples/jwks.json"
          cluster: "raw_githubusercontent_com_443"
          timeout {
            seconds: 5
          }
        }
        cache_duration {
          seconds: 300
        }
      }
      payload_in_metadata: "testing@secure.istio.io"
    }
  }
  rules {
    match {
      prefix: "/"
    }
    requirement_name: "requirement-0"
  }
  requirement_map {
    key: "requirement-0"
    value {
      provider_name: "example"
    }
  }
}
	{"runner": "xds-server"}

Let me know if see something I’m missing.

@danehans danehans force-pushed the authen_xds_trans branch 4 times, most recently from 15399b5 to 45c0cb8 Compare January 27, 2023 19:41
@danehans danehans marked this pull request as ready for review January 30, 2023 22:42
Comment thread internal/xds/translator/ratelimit.go Outdated
Comment thread internal/xds/translator/authentication.go Outdated
Comment thread internal/gatewayapi/filters.go Outdated
@danehans
Copy link
Copy Markdown
Contributor Author

danehans commented Jan 31, 2023

@arkodg thanks for the review. Commit 91e0b2b resolves your feedback above.

@danehans danehans requested review from arkodg and lizan January 31, 2023 03:29
@danehans danehans force-pushed the authen_xds_trans branch 3 times, most recently from a2f28c9 to 3830676 Compare February 1, 2023 19:44
Comment thread internal/xds/translator/translator.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

you for the case when two listeners have the same port, addFilterChain is false for the second listener, which means mgr is nil, which is being passed to this function.
rather than do this I suggest calling GetHTTPConnectionManagerFromListener here

func GetHTTPConnectionManagerFromListener(l *listener.Listener) (*hcm.HttpConnectionManager, error) {
	for _, filterChain := range l.FilterChains {
		for _, filter := range filterChain.Filters {
			if filter.Name == wellknown.HTTPConnectionManager {
				m := new(hcm.HttpConnectionManager)
				if err := filter.GetTypedConfig().UnmarshalTo(m); err != nil {
					return nil, err
				}
				return m, nil
			}
		}
	}
	return nil, ErrHcmNotFound
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've updated buildXdsRoute() to take the xds listener as a param. The authn code gets the hcm HTTP filters from the listener and creates the PerRouteConfig requirement for jwt.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

instead of using rIdx and fIdx in Gateway API, you could just use routeIdx and providerIdx here to build a unique providerKey

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this. Now that the IR no longer uses a keyed list of jwt providers, I removed the gatewayapi translator changes.

Signed-off-by: danehans <daneyonhansen@gmail.com>
@danehans danehans merged commit 4cf918d into envoyproxy:main Feb 1, 2023
@danehans danehans deleted the authen_xds_trans branch February 1, 2023 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/translator Issues related to Gateway's translation service, e.g. translating Gateway APIs into the IR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants