Skip to content

Conversation

@jacob-delgado
Copy link
Contributor

Commit Message:

...when using APPEND_FORWARD and SANITIZE set.

Currently XFCC only keeps the first URI in a certificate or a header presented
to it.

This works fine for most use cases, however, not everyone uses a single
URI (typically spiffe:// only).

Add every URI present to the 'By=' portion as well as the 'URI=' portion of an
XFCC header.

Additional Description:
Risk Level: Low
Testing: Unit and integration tests
Docs Changes: Yes. The appropriate http_connection_managers configuration section.
Release Notes: Yes
Fixes #20723

@jacob-delgado
Copy link
Contributor Author

The core requirement we are asked is to support is URI=. However, as I was reviewing the code I stumbled on By= also just looking at the code. I'm not sure if this was intentional or not.

I can put this behind a feature toggle if needed to get this merged. I would also like for this to be as a feature toggle in Envoy 1.19-1.21 as well, if possible. Thoughts?

...when using APPEND_FORWARD and SANITIZE set.

Currently XFCC only keeps the first URI in a certificate or a header presented
to it.

This works fine for most use cases, however, not everyone uses a single
URI (typically spiffe:// only).

Add every URI present to the 'By=' portion as well as the 'URI=' portion of an
XFCC header.

Signed-off-by: Jacob Delgado <jacob.delgado@volunteers.acasi.info>
Signed-off-by: Jacob Delgado <jacob.delgado@volunteers.acasi.info>
Signed-off-by: Jacob Delgado <jacob.delgado@volunteers.acasi.info>
@jacob-delgado
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #20724 (comment) was created by @jacob-delgado.

see: more, trace.

Copy link
Member

@lizan lizan left a comment

Choose a reason for hiding this comment

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

Looks good and I don't feel strongly we need a toggle for main branch.

Though I don't think we should backport either even with a toggle but backports are up to stable maintainer @pradeepcrao

@zuercher
Copy link
Member

/assign-from @envoyproxy/senior-maintainers

@repokitteh-read-only
Copy link

@envoyproxy/senior-maintainers assignee is @htuch

🐱

Caused by: a #20724 (comment) was created by @zuercher.

see: more, trace.

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

@lizan is senior maintainers, so will merge.

@htuch htuch merged commit b67a3fc into envoyproxy:main Apr 12, 2022
vehre-x41 pushed a commit to vehre-x41/envoy that referenced this pull request Apr 19, 2022
…xy#20724)

...when using APPEND_FORWARD and SANITIZE set.

Currently XFCC only keeps the first URI in a certificate or a header presented
to it.

This works fine for most use cases, however, not everyone uses a single
URI (typically spiffe:// only).

Add every URI present to the 'By=' portion as well as the 'URI=' portion of an
XFCC header.

Additional Description:
Risk Level: Low
Testing: Unit and integration tests
Docs Changes: Yes. The appropriate http_connection_managers configuration section.
Release Notes: Yes
Fixes envoyproxy#20723

Signed-off-by: Jacob Delgado <jacob.delgado@volunteers.acasi.info>
Signed-off-by: Andre Vehreschild <vehre@x41-dsec.de>
@jacob-delgado jacob-delgado deleted the xfcc-multiple-uri branch May 19, 2022 16:13
ravenblackx pushed a commit to ravenblackx/envoy that referenced this pull request Jun 8, 2022
…xy#20724)

...when using APPEND_FORWARD and SANITIZE set.

Currently XFCC only keeps the first URI in a certificate or a header presented
to it.

This works fine for most use cases, however, not everyone uses a single
URI (typically spiffe:// only).

Add every URI present to the 'By=' portion as well as the 'URI=' portion of an
XFCC header.

Additional Description:
Risk Level: Low
Testing: Unit and integration tests
Docs Changes: Yes. The appropriate http_connection_managers configuration section.
Release Notes: Yes
Fixes envoyproxy#20723

Signed-off-by: Jacob Delgado <jacob.delgado@volunteers.acasi.info>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

XFCC in APPEND_FORWARD or SANITIZE_SET only uses the first URI present

4 participants