envoy
envoy copied to clipboard
envoyproxy
SMTP Network Filter in contrib extensions
Commit Message: Smtp Network Filter in contrib Additional Description:
This smtp network filter has been developed as per RFC 5321 (https://www.rfc-editor.org/rfc/rfc5321) and supports STARTTLS extension. It will parse SMTP commands and responses as per the SMTP protocol and provide protocol specific metrics. The downstream starttls is supported by default. When smtp client sends STARTTLS, the filter will start secure transport on downstream connection. The upstream encryption is configurable. If enabled and if TLS handshake fails during upstream TLS negotiation, the filter responds to the client with error and closes the connection. If upstream TLS handshake succeeds, both the downstream and upstream connections are encrypted for secure smtp mail transfer.
Note: This new PR is created due to CI kickstart issues with another PR: https://github.com/envoyproxy/envoy/pull/25398
Risk Level: Low Testing: Unit Tests, Manual Testing Docs Changes: Release Notes: Platform Specific Features: No [Optional Runtime guard:] [Optional Fixes #Issue] https://github.com/envoyproxy/envoy/issues/24219 [Optional Fixes commit #PR or SHA] [Optional Deprecated:] [Optional API Considerations:]
Hi @VishalDamgude, welcome and thank you for your contribution.
We will try to review your Pull Request as quickly as possible.
In the meantime, please take a look at the contribution guidelines if you have not done so already.
:cat:
Caused by: https://github.com/envoyproxy/envoy/pull/26481 was opened by VishalDamgude.
![repokitteh-read-only[bot] avatar](https://avatars.githubusercontent.com/in/16729?v=4 "Published by a pub.dev verified publisher"){kind=small}
I'm happy that Vishal is taking the lead here but I would like to see a little more discussion about the requirements perhaps on #19765
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy[\w/]*/(v1alpha\d?|v1|v2alpha\d?|v2))|(api/envoy/type/(matcher/)?\w+.proto).
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).
:cat:
Caused by: https://github.com/envoyproxy/envoy/pull/26481 was synchronize by VishalDamgude.
@VishalDamgude any relationship between this PR and #27437? Seems there are some repeated content? cc @jsbucy
@VishalDamgude any relationship between this PR and #27437? Seems there are some repeated content? cc @jsbucy
@wbpcode Yes, the content seems repeated in #27437. The STARTTLS functionality (both upstream and downstream TLS support) is already implemented as part of this PR along with additional features like metrics, access logs and tracing.
@jsbucy if any additional functionality is required, can we please add it to this PR itself.
I will be addressing the review comments in this PR this week.
Hi Vishal, glad to connect! I wrote a doc detailing my assumptions for STARTTLS. I think the implementation in #27437 supports some more use cases that I think are important, in particular to sidecar with an smtp client or server (forward or reverse proxy) that doesn’t implement the STARTTLS extension.
One possible way forward would be to decouple the STARTTLS/session startup part from the deep protocol inspection/tracing part. Does the implementation here support the SMTP CHUNKING extension rfc3030?
Because these are lots of repeated content, could you figure out which one should be landed first? This is a huge work and no maintainer is familiar to the SMTP protocol, so, it's pretty hard for comunity to review both PRs or do a decision. cc @VishalDamgude @jsbucy
Vishal and I had a call last night to discuss, here are some general notes from the call, I'll add some more specific discussion topics in separate comments.
Vishal’s org is using this for reverse proxy with a custom SMTP server that doesn’t advertise many smtp extensions tracing/audit loging is a requirement for them
I’m fine with landing this PR first and iterating but I would like to try to avoid implementing backend protocol features that are completly unique to their application, (further comments to follow about x-req-id, starttls) I can port over the deltas from my PR once this has landed, in particular the support for additional starttls configs (upstream initiation for forward proxy, downstream termination transparent to the upstream) and the integration test
I think the current state of the tracing/deep protocol inspection code has some limitations and it will take some iteration to make it work with an MTA such as postfix that implements a typical suite of smtp extensions. I think it needs to be possible to disable protocol inspection after STARTTLS (corresponding to the PASSTHROUGH state in my PR) and that should be the default I think it’s fair to say firewalls have had a lot of trouble getting this right in the past one way this can go wrong is the inspector gets out of sync with the session and ends up interpreting an email message as smtp commands and hilarity ensues possibly the inspection code should have some heuristics to disable itself on too many unknown commands or if the server/upstream advertises an unknown/unsupported extension (I think PIPELINING, CHUNKING, AUTH PLAIN all definitely won’t work now) we should make sure something like AUTH PLAIN won’t end up logging the password as an unknown command in the envoy audit log!
would the postfix XCLIENT extension work for you instead of x-req-id? I was planning on adding this once we get basic starttls done
STARTTLS, in the current state of the code here:
- the server must advertise the capability
- the client must initiate STARTTLS command
- if client/downstream sends starttls and server/upstream returns a 2xx response, the filter enables downstream tls if upstream tls is configured, also does enables it there if upstream tls is disabled, this is basically a custom protocol where the server advertises the extension and returns a 2xx response but doesn’t actually do TLS? are you using it this way? does your application depend on the server receiving the starttls command? would passing the TLS info in XCLIENT work for you? do the possible starttls modes implemented in my PR meet your needs here?
cc @jsbucy Thanks very much for your hard work and response.
Then could you help to review this PR also? I think we still need some owner to maintain this extension after it's land.
cc @jsbucy Thanks very much for your hard work and response.
Then could you help to review this PR also? I think we still need some owner to maintain this extension after it's land.
Yeah I'll take another look at this PR.
I was expecting to be the owner if you don't mean an official envoy maintainer. The thing I really want to build is https://github.com/envoyproxy/envoy/issues/9133 for which starttls is basically the first step. I've been doing some prototyping to to figure out what that looks like before spending a lot of time building the production-grade envoy filter.
Attaching a document outlining all the features included in this PR and the proposed changes: https://docs.google.com/document/d/151tzHSvzs-xSyTJ7HvcepJsPTvO8mxVMNvnpAKDgmgg/edit?usp=sharing
cc: @jsbucy
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!
There are 2 open PRs for SMTP. Is there any concensus which one should go ahead? See https://github.com/envoyproxy/envoy/pull/27437.
Hi @cpakulski , We have discussed and decided to get this PR merged first. I am working on the comments, refactoring a bit. Will update this PR in few days.
cc: @jsbucy
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!