Expose new tlsMode and tlsVerify options for connecting to the mail submission agent #134

2025-12-14T23:22:55+01:00

ntninja commented

2025-12-14 23:22:55 +01:00

(Migrated from github.com)

Replaces #125.

Exposes all the cursed security options I’ve in the past or present seen mail servers require.

This is a slightly breaking change: If the mail server does not support any encryption, one has to now explicitly set email.tlsMode to "none", as the mail client will now refuse to automatically do an insecure downgrade to plaintext if StartTLS is not offered.

Replaces #125. Exposes all the cursed security options I’ve in the past or present seen mail servers require. This is a slightly breaking change: If the mail server does not support any encryption, one has to now explicitly set `email.tlsMode` to `"none"`, as the mail client will now refuse to automatically do an insecure downgrade to plaintext if StartTLS is not offered.

BrunoBernardino (Migrated from github.com) requested changes

2025-12-15 15:31:42 +01:00

BrunoBernardino (Migrated from github.com) left a comment

Thanks @ntninja this looks very good, but I don't want to ship a breaking change (especially so soon after one), can we come up with a way where that's not necessary?

Additionally, there's no need to change the .env.sample file; email is only used for signup or MFA, nothing else, and I don't plan to use it for anything else.

Thanks @ntninja this looks very good, but I don't want to ship a breaking change (especially so soon after one), can we come up with a way where that's not necessary? Additionally, there's no need to change the `.env.sample` file; email is only used for signup or MFA, nothing else, and I don't plan to use it for anything else.

ntninja commented

2025-12-15 17:17:57 +01:00

(Migrated from github.com)

Thanks @ntninja this looks very good, but I don't want to ship a breaking change (especially so soon after one), can we come up with a way where that's not necessary?

The only thing, I can think of is adding an additional value to tlsMode called "auto" that for bewCloud v3.x implements the new behaviour suggested here, while the null default retains the previous meaning. We could then emit a warning on startup if people have .email.port set to a value different from 465, but did not set .email.tlsMode.

For bewCloud 4.x null and "auto" will then become equivalent. (I could leave a PR open for that if you want.)

Sounds good enough?

There's no need to change the .env.sample file; email is only used for signup or MFA, nothing else, and I don't plan to use it for anything else.

The previous comment is wrong though: These variables do not (with or without this patch) enable signup email verification or multi-factor authentication via email.

Instead these variables have previously (and still do with this patch) only provided the authentication information when connecting the Mail Submission Agent. Not providing any authentication information is a better default than sending fake:fake as “credentials”.

I forgot to mention this, but without this patch connecting to the Mail Submission Agent is currently broken. This is because nodemailer will attempt PLAIN authentication if any .auth fields are present, but then the PLAIN authentication backend will (correctly) refuse to perform PAIN “authentication” without any credentials (doesn’t make sense and is, to my knowledge, prohibited by RFC).

> Thanks @ntninja this looks very good, but I don't want to ship a breaking change (especially so soon after one), can we come up with a way where that's not necessary? The only thing, I can think of is adding an additional value to `tlsMode` called `"auto"` that for bewCloud v3.x implements the new behaviour suggested here, while the `null` default retains the previous meaning. We could then emit a warning on startup if people have `.email.port` set to a value different from `465`, but did not set `.email.tlsMode`. For bewCloud 4.x `null` and `"auto"` will then become equivalent. (I could leave a PR open for that if you want.) Sounds good enough? > There's no need to change the `.env.sample` file; email is only used for signup or MFA, nothing else, and I don't plan to use it for anything else. The previous comment is wrong though: These variables do not (with or without this patch) *enable* signup email verification or multi-factor authentication via email. Instead these variables have previously (and still do with this patch) only provided the authentication information when connecting the Mail Submission Agent. Not providing any authentication information is a better default than sending `fake:fake` as “credentials”. I forgot to mention this, but without this patch connecting to the Mail Submission Agent is currently broken. This is because nodemailer will attempt `PLAIN` authentication if *any* `.auth` fields are present, but then the `PLAIN` authentication backend will (correctly) refuse to perform `PAIN` “authentication” without any credentials (doesn’t make sense and is, to my knowledge, prohibited by RFC).

BrunoBernardino commented

2025-12-15 17:38:02 +01:00

(Migrated from github.com)

Sounds good enough?

I might be missing something, but why can't we not allow null, default to auto, and have auto behave as it does now?

The previous comment is wrong though

I suppose you can change enable to use, then.

> Sounds good enough? I might be missing something, but why can't we not allow `null`, default to `auto`, and have `auto` behave as it does now? > The previous comment is wrong though I suppose you can change `enable` to `use`, then.

ntninja commented

2025-12-15 17:46:41 +01:00

(Migrated from github.com)

Sounds good enough?

I might be missing something, but why can't we not allow null, default to auto, and have auto behave as it does now?

Because null as implemented here, mean “immediate TLS on port 465, required StartTLS otherwise”, while the previous default in bewCloud is “immediate TLS on port 465, opportunistic StartTLS otherwise”.

The proposed mechanism is for “auto” (the new recommended value mentioned in sample config) to mean “immediate TLS on port 465, required StartTLS otherwise”, with the default null retaining the previous bewCloud behaviour of “immediate TLS on port 465, opportunistic StartTLS otherwise”. They can then become one and the same in bewCloud 4.

The null default value is intended is an indefinite stop-gap for backward-compatibility during bewCloud 3 only.

> > Sounds good enough? > > I might be missing something, but why can't we not allow `null`, default to `auto`, and have `auto` behave as it does now? Because `null` as implemented here, mean “immediate TLS on port 465, *required* StartTLS otherwise”, while the previous default in bewCloud is “immediate TLS on port 465, *opportunistic* StartTLS otherwise”. The proposed mechanism is for “auto” (the new recommended value mentioned in sample config) to mean “immediate TLS on port 465, *required* StartTLS otherwise”, with the default `null` retaining the previous bewCloud behaviour of “immediate TLS on port 465, *opportunistic* StartTLS otherwise”. They can then become one and the same in bewCloud 4. The `null` default value is intended is an indefinite stop-gap for backward-compatibility during bewCloud 3 only.

ntninja commented

2025-12-15 17:50:59 +01:00

(Migrated from github.com)

The previous comment is wrong though

I suppose you can change enable to use, then.

So would this be OK then?

#SMTP_USERNAME=""  # optional, if you want to use signup email verification or multi-factor with an email service requiring authentication
#SMTP_PASSWORD=""  # optional, if you want to use signup email verification or multi-factor with an email service requiring authentication

> > The previous comment is wrong though > > I suppose you can change `enable` to `use`, then. So would this be OK then? ```env #SMTP_USERNAME="" # optional, if you want to use signup email verification or multi-factor with an email service requiring authentication #SMTP_PASSWORD="" # optional, if you want to use signup email verification or multi-factor with an email service requiring authentication ```

ntninja commented

2025-12-15 17:58:42 +01:00

(Migrated from github.com)

@BrunoBernardino: Commit https://github.com/bewcloud/bewcloud/pull/134/changes/471a1828ea0fbf9db9f7aee93979ad6ab678891a implements the legacy compatibility behaviour for tlsMode.

@BrunoBernardino: Commit https://github.com/bewcloud/bewcloud/pull/134/changes/471a1828ea0fbf9db9f7aee93979ad6ab678891a implements the legacy compatibility behaviour for `tlsMode`.

BrunoBernardino (Migrated from github.com) requested changes

2025-12-15 20:24:19 +01:00

BrunoBernardino (Migrated from github.com) left a comment

This is looking great and very close! I have a few minor tweaks and a slight refactor request!

bewcloud.config.sample.ts

				`@ -32,6 +32,8 @@ const config: PartialDeep<Config> = {`
				`// from: '[email protected]',`

This should match the type JSDoc:

  //  "auto" means "immediate" on port 465, "starttls" otherwise; `null` is legacy behaviour and will be removed in v4; it means "immediate" if port is 465, but opportunistic StartTLS or plain transmission otherwise.

This should match the type JSDoc: ```suggestion // "auto" means "immediate" on port 465, "starttls" otherwise; `null` is legacy behaviour and will be removed in v4; it means "immediate" if port is 465, but opportunistic StartTLS or plain transmission otherwise. ```

This should match the type JSDoc:

  //  Whether to verify the TLS certificate. A hostname can be used instead of true/false as well.

This should match the type JSDoc: ```suggestion // Whether to verify the TLS certificate. A hostname can be used instead of true/false as well. ```

lib/config.ts

					
				@ -37,6 +37,8 @@ export class AppConfig {

				        from: '[email protected]',

I think this comment is confusing and unnecessary since we have more info in the types/JSDoc.

        tlsMode: null,

I think this comment is confusing and unnecessary since we have more info in the types/JSDoc. ```suggestion tlsMode: null, ```

lib/config.ts

				`@ -97,6 +99,10 @@ export class AppConfig {`

Please make this more consistent with past warnings.

        console.warn('DEPRECATION WARNING: When using `config.email.port` with a value other than `465`, please set `config.email.tlsMode` to either `'starttls'` or `'none'` to explicitly enable or disable usage of StartTLS! Support for legacy opportunistic StartTLS will be removed in bewCloud 4!');

Please make this more consistent with past warnings. ```suggestion console.warn('DEPRECATION WARNING: When using `config.email.port` with a value other than `465`, please set `config.email.tlsMode` to either `'starttls'` or `'none'` to explicitly enable or disable usage of StartTLS! Support for legacy opportunistic StartTLS will be removed in bewCloud 4!'); ```

lib/models/email.ts

					
				@ -15,14 +15,31 @@ export class EmailModel {

				      throw new Error('config.email.from, config.email.host, or config.email.port is not set');

				    }

				    let tlsMode = emailConfig.tlsMode;

Please rewrite this bit to remain/keep the transporterConfig a variable that's just used in createTransport later. It makes debugging configuration easier in the future.

Please rewrite this bit to remain/keep the `transporterConfig` a variable that's just used in `createTransport` later. It makes debugging configuration easier in the future.

lib/types.ts

					
				@ -206,6 +206,10 @@ export interface Config {

				    host: string;

I think this can be simplified.

    /** "auto" means "immediate" on port 465, "starttls" otherwise; `null` is legacy behaviour and will be removed in v4; it means "immediate" if port is 465, but opportunistic StartTLS or plain transmission otherwise. */

I think this can be simplified. ```suggestion /** "auto" means "immediate" on port 465, "starttls" otherwise; `null` is legacy behaviour and will be removed in v4; it means "immediate" if port is 465, but opportunistic StartTLS or plain transmission otherwise. */ ```

I think this can be clearer.

    /** Whether to verify the TLS certificate. A hostname can be used instead of true/false as well. */

I think this can be clearer. ```suggestion /** Whether to verify the TLS certificate. A hostname can be used instead of true/false as well. */ ```

ntninja (Migrated from github.com) reviewed

2025-12-17 22:50:44 +01:00

lib/types.ts

					
				@ -206,6 +206,10 @@ export interface Config {

				    host: string;

Changed it in a slightly way, yous makes it non-obvious that the hostname is indeed only for TLS certificate verification

ntninja (Migrated from github.com) reviewed

2025-12-17 22:52:37 +01:00

lib/models/email.ts

					
				@ -15,14 +15,31 @@ export class EmailModel {

				      throw new Error('config.email.from, config.email.host, or config.email.port is not set');

				    }

				    let tlsMode = emailConfig.tlsMode;

Makes sense, I had even done that during debugging… 🤔

Makes sense, I had even done that during debugging… :thinking:

ntninja commented

2025-12-18 01:34:01 +01:00

(Migrated from github.com)

@BrunoBernardino: Should hopefully be OK now

BrunoBernardino (Migrated from github.com) requested changes

2025-12-18 19:12:45 +01:00

BrunoBernardino (Migrated from github.com) left a comment

Almost there, I just have a couple more suggestions and questions! Thank you for your patience!

lib/models/email.ts

					
				@ -15,14 +15,31 @@ export class EmailModel {

				      throw new Error('config.email.from, config.email.host, or config.email.port is not set');

We don't need to split comments into multiple lines, and I personally don't like it.

      // Value “default” will be ignored below causing the nodemailer default behaviour of using opportunistic StartTLS

We don't need to split comments into multiple lines, and I personally don't like it. ```suggestion // Value “default” will be ignored below causing the nodemailer default behaviour of using opportunistic StartTLS ```

lib/models/email.ts

					
				@ -23,0 +33,4 @@

				      tls: (

				        emailConfig.tlsVerify === false ? { rejectUnauthorized: false } :

				        emailConfig.tlsVerify !== true  ? { servername: emailConfig.tlsVerify } :

				        {}

Shouldn't we set rejectUnauthorized: true if tlsVerify === true?

Shouldn't we set `rejectUnauthorized: true` if `tlsVerify === true`?

lib/models/email.ts

					
				@ -23,0 +35,4 @@

				        emailConfig.tlsVerify !== true  ? { servername: emailConfig.tlsVerify } :

				        {}

				      ),

Isn't it better to set this as undefined or null instead of an empty object? I haven't checked the nodemailer config code yet, but I'd assume null given what's done for auth below.

Isn't it better to set this as `undefined` or `null` instead of an empty object? I haven't checked the `nodemailer` config code yet, but I'd assume `null` given what's done for `auth` below.

lib/types.ts

					
				@ -206,6 +206,10 @@ export interface Config {

				    host: string;

Pedantic, but makes it more understandable.

    /** Whether to verify the TLS certificate, if a string is used, the hostname will be verified using that name */

Pedantic, but makes it more understandable. ```suggestion /** Whether to verify the TLS certificate, if a string is used, the hostname will be verified using that name */ ```

ntninja (Migrated from github.com) reviewed

2025-12-19 21:23:58 +01:00

lib/models/email.ts

					
				@ -23,0 +33,4 @@

				      tls: (

				        emailConfig.tlsVerify === false ? { rejectUnauthorized: false } :

				        emailConfig.tlsVerify !== true  ? { servername: emailConfig.tlsVerify } :

				        {}

Not necessary, unlike with opportunistic StartTLS (which makes things “just work” more often, but also reduces security in the face of an active attacker to exactly 0) there are no bad defaults here. (That is, TLS certificates are always checked unless you explicitly opt out of that.)

ntninja (Migrated from github.com) reviewed

2025-12-19 21:26:46 +01:00

lib/models/email.ts

					
				@ -23,0 +35,4 @@

				        emailConfig.tlsVerify !== true  ? { servername: emailConfig.tlsVerify } :

				        {}

				      ),

Empty object means: Use the defaults, so this is OK.
(auth is different: The presence of any object signals it to enable authentication, so I had to pass null there – not a great API.)

Empty object means: Use the defaults, so this is OK. (`auth` is different: The presence of any object signals it to enable authentication, so I had to pass `null` there – not a great API.)

ntninja (Migrated from github.com) reviewed

2025-12-19 21:32:00 +01:00

lib/types.ts

					
				@ -206,6 +206,10 @@ export interface Config {

				    host: string;

That’s incorrect grammar in english I’m afraid, you don’t add the comma its clear that the if and the are part of the same clause. How you suggested may also be ready as “Whether to verify the TLS certificate, if a string is used” + [something]. It’s like the famous gun amendment of the US constitution. 😆

That’s incorrect grammar in english I’m afraid, you don’t add the comma its clear that the if and the are part of the same clause. How you suggested may also be ready as “Whether to verify the TLS certificate, if a string is used” + [something]. It’s like the famous gun amendment of the US constitution. :laughing:

ntninja commented

2025-12-19 21:32:53 +01:00

(Migrated from github.com)

Fixed the multi-line comment

BrunoBernardino (Migrated from github.com) reviewed

2025-12-20 12:43:56 +01:00

lib/models/email.ts

					
				@ -23,0 +33,4 @@

				      tls: (

				        emailConfig.tlsVerify === false ? { rejectUnauthorized: false } :

				        emailConfig.tlsVerify !== true  ? { servername: emailConfig.tlsVerify } :

				        {}

Thanks for the clarification!

BrunoBernardino (Migrated from github.com) reviewed

2025-12-20 12:44:27 +01:00

lib/models/email.ts

					
				@ -23,0 +35,4 @@

				        emailConfig.tlsVerify !== true  ? { servername: emailConfig.tlsVerify } :

				        {}

				      ),

Gotcha.

BrunoBernardino (Migrated from github.com) reviewed

2025-12-20 12:46:07 +01:00

lib/types.ts

					
				@ -206,6 +206,10 @@ export interface Config {

				    host: string;

I should've made the first comma a dot, sorry.

BrunoBernardino (Migrated from github.com) reviewed

2025-12-20 12:46:35 +01:00

lib/types.ts

					
				@ -206,6 +206,10 @@ export interface Config {

				    host: string;

Like this:

    /** Whether to verify the TLS certificate. If a string is used, the hostname will be verified using that name */

Like this: ```suggestion /** Whether to verify the TLS certificate. If a string is used, the hostname will be verified using that name */ ```

BrunoBernardino (Migrated from github.com) approved these changes

2025-12-20 12:49:35 +01:00

BrunoBernardino (Migrated from github.com) left a comment

Thanks for the changes!

BrunoBernardino commented

2025-12-20 13:07:08 +01:00

(Migrated from github.com)

This is live in v3.3.0!

This is live in [v3.3.0](https://github.com/bewcloud/bewcloud/releases/tag/v3.3.0)!

Sign in to join this conversation.

No reviewers

BrunoBernardino

No labels

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

bewcloud/bewcloud!134

				`@ -37,6 +37,8 @@ export class AppConfig {`
				`from: '[email protected]',`

				`@ -206,6 +206,10 @@ export interface Config {`
				`host: string;`

				`@ -15,14 +15,31 @@ export class EmailModel {`
				`throw new Error('config.email.from, config.email.host, or config.email.port is not set');`

Rows
Columns

Expose new tlsMode and tlsVerify options for connecting to the mail submission agent #134

Expose new `tlsMode` and `tlsVerify` options for connecting to the mail submission agent #134