[#246] Make STARTTLS mandatory: support starttls.required in external.email

[vharseko]

Fixes #246

Problem

STARTTLS in external.email is opportunistic. If the server's EHLO reply does not offer STARTTLS, javax.mail 1.4.7 goes on in clear unless mail.smtp.starttls.required is set. An on-path attacker who strips STARTTLS therefore receives the SMTP credentials and the message, and the certificate checks from #206 never run.

Change

• EmailClient: new optional starttls.required (default false), mapped to mail.smtp.starttls.required.
  • required implies enable. javax.mail 1.4.7's SMTPTransport issues STARTTLS when useStartTLS || requireStartTLS (checked in the bytecode). Without the implication, {enable: false, required: true} would run STARTTLS without the trust settings from Harden GitHub workflows and SMTP STARTTLS trust #206, that is, without the host-name check.
• Default false, so servers without STARTTLS keep working. Both samples/*/external.email.json (smtp.gmail.com:587) and the example in chap-mail.adoc set "required" : true.
• Admin UI: a "Require STARTTLS" switch next to "Use STARTTLS".
  • Turning it on turns STARTTLS on, and turning STARTTLS off clears it.
  • A stored {"required": true} without enable renders "Use STARTTLS" on, matching what EmailClient applies.
  • An unchecked switch is saved as false, because form2js returns false for an unchecked value="true" checkbox, so the starttls merge from Harden GitHub workflows and SMTP STARTTLS trust #206 cannot keep a stale true.
• chap-mail.adoc documents required, the downgrade it prevents, and that it implies enable.

Tests

• EmailClientTest:
  • Property-level cases: the default is false, required sets the property, and required implies enable and checkserveridentity.
  • Two cases against an in-process SMTP server whose EHLO reply omits STARTTLS:
    • With required, the send fails with "STARTTLS is required" and the server never sees MAIL FROM.
    • Without it, the message goes out in clear. This pins the downgrade described in Make STARTTLS mandatory: support starttls.required in external.email #246.
  • Mutation check: dropping the mail.smtp.starttls.required put turns 4 cases red, including the server case ("sent over a connection without STARTTLS").
• pom.xml: test-scoped com.sun.activation:javax.activation:1.2.0. jaxb-api brings in only javax.activation-api, which has no implementation classes, so send() in a test failed with NoClassDefFoundError: com/sun/activation/registries/LogSupport.
• EmailConfigViewTest.js:
  • Two cases render the real EmailConfigTemplate.html: a stored required alone shows "Use STARTTLS" on, and unchecking the rendered "Require STARTTLS" switch saves required: false over a stored true.
  • The switches are driven through change events, so the view's events map is covered.
  • Mutation check: dropping the required fallback on "Use STARTTLS", dropping value="true" or renaming name="starttls.required" on the Require switch, and deleting either change binding each turn one case red.
• Local runs on the branch rebased onto master: openidm-external-email 17/17, openidm-ui-common build green, and the openidm-ui-admin build (eslint, QUnit 127/127) green.

[maximthomas]

Review action: Approve — mergeable as is; one cosmetic UI issue and two test pins, all non-blocking. Reviewed the last commit, 2ab4ef0, as the description asks; #206's commits are not reviewed here.

---

praise: The fix sits where the downgrade is and closes the road JavaMail would otherwise open.

• required implies enable in EmailClient.java:106-110, so the #206 trust settings and host check also apply when only required is set, the case JavaMail 1.4.7 would otherwise run STARTTLS without them.
• startTlsRequiredStopsBeforeMailWhenTheServerDoesNotOfferIt and startTlsWithoutRequiredSendsInClearWhenTheServerDoesNotOfferIt drive a real send against StartTlsStrippingServer; they also run on the Windows leg (build-maven windows-latest 26: 17/17 in the module).
• value="true" on the new switch makes form2js save an unchecked switch as false, so the #206 starttls merge cannot keep a stale true.

---

issue (non-blocking): A stored required: true without enable: true renders "Use STARTTLS" off.

openidm-ui/openidm-ui-admin/src/main/resources/templates/admin/settings/EmailConfigTemplate.html:58, :69

The template checks config.starttls.enable and config.starttls.required separately, and syncStartTls runs only on change (setup() triggers validate), so nothing reconciles them at render. "starttls": {"required": true} — valid per chap-mail.adoc, which says required implies enable — shows "Use STARTTLS" off and "Require STARTTLS" on, while EmailClient runs STARTTLS. The UI shows a state the server does not apply. Saving untouched keeps required: true, so nothing is downgraded.

<input type="checkbox" name="starttls.enable" id="emailTLS" value="true" {{#if config.starttls.enable}}checked{{else}}{{#if config.starttls.required}}checked{{/if}}{{/if}}/><span></span>

---

suggestion (non-blocking): The save test's hand-written fixture does not pin the template's name="starttls.required" value="true".

openidm-ui/openidm-ui-admin/src/test/qunit/org/forgerock/openidm/ui/admin/settings/EmailConfigViewTest.js:54, openidm-ui/openidm-ui-admin/src/main/resources/templates/admin/settings/EmailConfigTemplate.html:69

save() is unchanged by this commit, so the case exercises #206's merge and is green at the parent a997a14. Dropping value="true" or renaming name on template line 69 leaves it green. In the real form, form2js then skips the unchecked switch, the merge at EmailConfigView.js:152 keeps a stored required: true, and the admin cannot clear it — the stale-true case the description says is prevented. Not run: whether parentRender can fetch the template in the grunt-qunit page.

QUnit.test("the rendered Require STARTTLS switch saves false when unchecked", function (assert) {
    var done = assert.async(), saved,
        read = sinon.stub(ConfigDelegate, "readEntity", function () {
            return $.Deferred().resolve({ host: "smtp.example.com", starttls: { enable: true, required: true } });
        }),
        update = sinon.stub(ConfigDelegate, "updateEntity", function (id, config) { saved = config; return $.Deferred(); });
    $("#qunit-fixture").html('<div id="emailContainer"></div>');
    EmailConfigView.render([], function () {
        EmailConfigView.$el.find("#emailTLSRequired").prop("checked", false);
        EmailConfigView.save({ preventDefault: $.noop });
        read.restore(); update.restore();
        assert.strictEqual(saved.starttls.required, false, "the template's switch reaches the saved config");
        done();
    });
});

Pin: the case turns red when value="true" is dropped or name="starttls.required" is renamed on template line 69.

---

suggestion (non-blocking): The new change #emailTLS / change #emailTLSRequired bindings are not tested.

openidm-ui/openidm-ui-admin/src/main/js/org/forgerock/openidm/ui/admin/settings/EmailConfigView.js:40-41

'STARTTLS required implies STARTTLS in the form' calls syncStartTls by hand, and no admin QUnit case delegates a view's events map. Deleting both events entries leaves every case green, and in the real form ticking "Require STARTTLS" would leave "Use STARTTLS" off. Not run.

QUnit.test("the STARTTLS switches stay consistent on change", function (assert) {
    $("#qunit-fixture").html('<input type="checkbox" id="emailTLS">' +
        '<input type="checkbox" id="emailTLSRequired">');
    EmailConfigView.setElement($("#qunit-fixture"));
    $("#emailTLSRequired").prop("checked", true).trigger("change");
    assert.ok($("#emailTLS").prop("checked"), "checking required turns STARTTLS on");
    $("#emailTLS").prop("checked", false).trigger("change");
    assert.notOk($("#emailTLSRequired").prop("checked"), "turning STARTTLS off clears required");
    EmailConfigView.undelegateEvents();
});

Pin: the case turns red when either change entry at EmailConfigView.js:40-41, or the id it selects, is deleted or misspelled.

[vharseko]

@maximthomas All three points are addressed in def91dd. #206 is merged, so the branch is rebased onto master and now holds only this PR's two commits.

issue: a stored required: true without enable showed "Use STARTTLS" off. Fixed in EmailConfigTemplate.html with the fallback you proposed: "Use STARTTLS" is checked when config.starttls.enable or config.starttls.required is set. After an untouched save the stored config gets enable: true, which is what EmailClient already applies.

suggestion: the save case did not pin the template's name="starttls.required" value="true". The hand-written fixture is replaced by cases that render the real template through EmailConfigView.render():

• a stored STARTTLS required renders Use STARTTLS on covers the issue above.
• the rendered Require STARTTLS switch saves false when unchecked unchecks the rendered switch, saves, and asserts required: false over the stored true.

parentRender does fetch the template in the grunt-qunit page (file:// with --allow-file-access-from-files). The cases stub ThemeManager.getTheme and ValidatorsManager.bindValidators/validateAllFields. Without the theme stub, the readEntity stub answers ui/themeconfig too and loadThemeCSS throws. Without the validators stubs, toggleEmail() fails on the unconfigured validators.

suggestion: the change bindings were not tested. the STARTTLS switches stay consistent on change replaces the direct syncStartTls calls. It uses setElement and trigger("change"), as you proposed, and calls undelegateEvents() at the end.

Mutation checks against the built target/www. Each one turns exactly one case red, and restoring the files brings back 127/127:

Mutation	Red case
drop the required fallback on "Use STARTTLS"	renders Use STARTTLS on
drop value="true" on the Require switch	saves false when unchecked
rename name="starttls.required"	saves false when unchecked
delete both change entries	switches stay consistent on change
delete only change #emailTLS	switches stay consistent on change
delete only change #emailTLSRequired	switches stay consistent on change

Local runs: openidm-ui-admin build (eslint, QUnit 127/127) green, openidm-external-email 17/17.

[maximthomas]

praise: The change sits where the downgrade happens, and the tests are measured, not asserted.

• EmailClient (EmailClient.java:106-114): required implies enable and maps to mail.smtp.starttls.required, so {enable: false, required: true} still gets the #206 trust settings.
• EmailConfigTemplate.html:58 now renders "Use STARTTLS" on for a stored required-only config, which matches what EmailClient applies. This closes the round-1 render mismatch.
• The mutation claims hold when run. Deleting the mail.smtp.starttls.required put turns 4 EmailClientTest cases red, the STARTTLS-stripping server case among them. Dropping startTLSRequired || turns startTlsRequiredImpliesStartTls red. Each of the template fallback, the Require switch's value="true" and the change #emailTLSRequired binding, when removed, turns exactly one EmailConfigViewTest case red (127 run, 1 failed each).

[vharseko]

Merged the current master (4ca6a39ac) into the branch in be27a3838 to pick up #249. CI on the previous head def91dd5a failed only because master itself did not compile at that point: openidm-repo-orientdb stopped at DocumentUtil.java:[106,33] cannot find symbol: variable topLevel after #245, in every build-maven job, and both Docker jobs then had no artifact to download.

The PR's own two commits are untouched, and the merge brings no change of its own: its tree is identical to a rebase of the two commits onto 4ca6a39ac. CI is running on the new head.