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

Update VirtualServer to ignore CRL for EgressMTLS #3737

Merged
merged 13 commits into from
Apr 26, 2023
Merged

Update VirtualServer to ignore CRL for EgressMTLS #3737

merged 13 commits into from
Apr 26, 2023

Conversation

shaun-nx
Copy link
Contributor

@shaun-nx shaun-nx commented Apr 7, 2023

Proposed changes

This change updates the logic which applies policies to VirtualServer to ignore CRL for EgressMTLS.
Resolves #3732

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

@shaun-nx shaun-nx requested a review from a team as a code owner April 7, 2023 17:21
@github-actions github-actions bot added bug An issue reporting a potential bug tests Pull requests that update tests labels Apr 7, 2023
@codecov
Copy link

codecov bot commented Apr 11, 2023

Codecov Report

Merging #3737 (4c36d81) into main (38ae409) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3737      +/-   ##
==========================================
+ Coverage   52.37%   52.41%   +0.03%     
==========================================
  Files          59       59              
  Lines       16898    16902       +4     
==========================================
+ Hits         8851     8859       +8     
+ Misses       7750     7748       -2     
+ Partials      297      295       -2     
Impacted Files Coverage Δ
internal/configs/virtualserver.go 95.17% <100.00%> (+0.01%) ⬆️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@brianehlert brianehlert added this to the v3.1.1 milestone Apr 17, 2023
Copy link
Member

@lucacome lucacome left a comment

Choose a reason for hiding this comment

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

I don't think we want to ignore the CRL. We should use the right directive for it proxy_ssl_crl.

@brianehlert
Copy link
Collaborator

I don't think we want to ignore the CRL. We should use the right directive for it proxy_ssl_crl.

That would be the right thing to do.

@shaun-nx
Copy link
Contributor Author

@brianehlert @lucacome
The aim of this PR isn't to support using CRLs with EgressMTLs. This only addresses the issue posted in #3732

We only added support for CRLs with IngressMTLs but not EgressMTLs.
The PR for IngressMTLs update is here for reference: #3632

@brianehlert
Copy link
Collaborator

To close this conversation, we will be adding proper EgressMTLS support for CRL to match IngressMTLS behavior.
That is not work contained in this PR.

@shaun-nx shaun-nx requested a review from lucacome April 25, 2023 05:12
@lucacome lucacome dismissed their stale review April 26, 2023 02:10

avoid blocking PR

@shaun-nx shaun-nx merged commit 87b8a58 into main Apr 26, 2023
@shaun-nx shaun-nx deleted the fix/caSecret branch April 26, 2023 08:58
lucacome pushed a commit that referenced this pull request May 4, 2023
* Update VirtualServer to ignore CRL for EgressMTLS

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Un-comment tests

* Fix crt and crl path in test and fix nill slice reference

* Update data files for egress MTLS tests

* Remove VSR python test

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Add new app.yaml file for EgressMTLS tests

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
(cherry picked from commit 87b8a58)
lucacome added a commit that referenced this pull request May 4, 2023
Update VirtualServer to ignore CRL for EgressMTLS (#3737)

* Update VirtualServer to ignore CRL for EgressMTLS

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Un-comment tests

* Fix crt and crl path in test and fix nill slice reference

* Update data files for egress MTLS tests

* Remove VSR python test

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Add new app.yaml file for EgressMTLS tests

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
(cherry picked from commit 87b8a58)

Co-authored-by: Shaun <s.odonovan@f5.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue reporting a potential bug tests Pull requests that update tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

proxy_ssl_trusted_certificate getting cert and then crl on the same line
6 participants