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

[40] Build SMTP Integration For Threats Logging #201

Conversation

azenna
Copy link
Contributor

@azenna azenna commented Sep 13, 2023

Build SMTP Integration for threats logging

Adds a smtp notifier module that sends email on threats. Uses mail-sender library for convenience, could write inhouse smtp-client if necessary.

I have

  • run cargo fmt;
  • run cargo clippy;
  • run cargo testand all tests pass;
  • linked to the originating issue (if applicable).

resolves #40

@azenna azenna force-pushed the 40-build-smtp-integration-for-threats-logging branch 2 times, most recently from ea85c92 to 25476e6 Compare September 14, 2023 03:46
Copy link
Member

@vadorovsky vadorovsky left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! The code in general looks good, I left few minor comments.

crates/modules/smtp-notifier/Cargo.toml Outdated Show resolved Hide resolved
[dependencies]
pulsar-core = { path = "../../pulsar-core" }

tokio = { version = "1", features = ["full"] }
Copy link
Member

Choose a reason for hiding this comment

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

Hmm... can we double-check whether we really need the full set of features? Maybe it's possible to use just specific ones?

crates/modules/smtp-notifier/README.md Outdated Show resolved Hide resolved
@vadorovsky
Copy link
Member

The CI failure seems legit - the ring crate fails to build on riscv. We need to make sure we don't build it for riscv. I will think of solutions and let you know of I have any ideas, but feel free to also look for solutions. :)

@azenna azenna force-pushed the 40-build-smtp-integration-for-threats-logging branch from 25476e6 to 814fcc8 Compare September 14, 2023 09:09
@azenna
Copy link
Contributor Author

azenna commented Sep 14, 2023

Hmm I think these can maybe be fixed by adding libsssl-dev to the CI. Thoughts?

@azenna azenna force-pushed the 40-build-smtp-integration-for-threats-logging branch from 814fcc8 to c878abb Compare September 14, 2023 09:23
@banditopazzo
Copy link
Member

Hi, I think a better solution is to use lettre, it's a more advanced crate. by default it uses openssl instead of rustls, so it should compile on riscv64

@azenna azenna force-pushed the 40-build-smtp-integration-for-threats-logging branch from c878abb to d7c72c4 Compare September 14, 2023 09:34
Comment on lines 70 to 133
struct SmtpNotifierConfig {
server: String,
user: String,
password: String,
receiver: String,
}
Copy link
Member

Choose a reason for hiding this comment

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

you should also add port in the configuration and an option for the encryption, something like:

enum Encryption {
     Plain,
     TLS,
     StartTLS
}

@banditopazzo
Copy link
Member

you need to add libssl-dev in dockerfiles under .cross, not in the github workflow and expose a openssl-vendored feature for musl builds

@azenna azenna force-pushed the 40-build-smtp-integration-for-threats-logging branch 2 times, most recently from e6530ff to 07d06ea Compare September 14, 2023 11:20
@azenna
Copy link
Contributor Author

azenna commented Sep 14, 2023

Got everything building 🎉 Added the tls and port configuration options as well

Copy link
Member

@vadorovsky vadorovsky left a comment

Choose a reason for hiding this comment

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

Looks great, thanks! I will let @banditopazzo to give a final approval

Copy link
Member

@banditopazzo banditopazzo left a comment

Choose a reason for hiding this comment

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

the main issue is the fixed vendored feature of openssl: in normal cases openssl should be dynamically linked with the option to link it statically, for example with musl. this should be under a feature and the feature should be re-exported by pulsar binary.
I added few suggestions to cross containers and tomls but needs to double checked.

the workflows need to be modified for musl builds adding an extra feature openssl-vendored

.cross/aarch64-unknown-linux-gnu.Dockerfile Outdated Show resolved Hide resolved
.cross/aarch64-unknown-linux-gnu.Dockerfile Outdated Show resolved Hide resolved
.cross/aarch64-unknown-linux-musl.Dockerfile Outdated Show resolved Hide resolved
.cross/aarch64-unknown-linux-musl.Dockerfile Outdated Show resolved Hide resolved
.cross/riscv64gc-unknown-linux-gnu.Dockerfile Outdated Show resolved Hide resolved
crates/modules/smtp-notifier/Cargo.toml Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
crates/modules/smtp-notifier/Cargo.toml Outdated Show resolved Hide resolved
Comment on lines 57 to 61
let message = Message::builder()
.from("Pulsar <pulsar-threat-notification@gmail.com>".parse()?)
.to(config.receiver.parse()?)
.subject(subject)
.body(body)?;
Copy link
Member

Choose a reason for hiding this comment

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

the .from in the builder should be optional. for many email providers it's not possible to change the sender because of authentication implications and some of them don't require it to be set

crates/modules/smtp-notifier/src/lib.rs Outdated Show resolved Hide resolved
@azenna azenna force-pushed the 40-build-smtp-integration-for-threats-logging branch 6 times, most recently from ad52979 to 717c4d4 Compare September 14, 2023 21:17
@azenna
Copy link
Contributor Author

azenna commented Sep 14, 2023

the main issue is the fixed vendored feature of openssl: in normal cases openssl should be dynamically linked with the option to link it statically, for example with musl. this should be under a feature and the feature should be re-exported by pulsar binary. I added few suggestions to cross containers and tomls but needs to double checked.

the workflows need to be modified for musl builds adding an extra feature openssl-vendored

Couldn't figure out how to pass a --feature arg to cross from the CI, but did find an alternative using platform specific dependencies targeting target_env: musl, gnu

@azenna azenna force-pushed the 40-build-smtp-integration-for-threats-logging branch from 717c4d4 to 310ad3a Compare September 14, 2023 22:12
@banditopazzo
Copy link
Member

#202 is merged, please rebase on main to check if the ci passes after those changes

@azenna azenna force-pushed the 40-build-smtp-integration-for-threats-logging branch from 310ad3a to f7de67a Compare September 16, 2023 14:43
@azenna azenna force-pushed the 40-build-smtp-integration-for-threats-logging branch from f7de67a to c074c56 Compare September 16, 2023 14:56
Copy link
Member

@banditopazzo banditopazzo left a comment

Choose a reason for hiding this comment

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

Few minor changes are necessary, I will make a fix after this is merged

@banditopazzo banditopazzo merged commit b6bf731 into exein-io:main Sep 18, 2023
17 checks passed
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.

build SMTP integration for threats logging
3 participants