Repository navigation
Allow and control sending UTF-8 emails in the default email sender impl - #41715
Conversation
There was a problem hiding this comment.
The PR has this setting now to "false", which might lead to some emails no longer being handed over to the SMTP-server.
So I wonder:
- Should the default be "true" for new SMTP configurations?
- Is this a notable change, that should mention the new option, as those migrating should review their configuration and set it to "true"?
Something that is IMHO missing:
- If we validate the "from" address like above, the validation should also be performed when we save the configuration.
Something that we could test:
- Does the server support UTF8 - this can be tested by the following - which unfortunately calls the implementation, and not the interface. The Angus mail implementation only logs an INFO is there is a mismatch.
((SMTPTransport) transport).supportsExtension("SMTPUTF8")
See some more comments below. Feel free to split those ...
There was a problem hiding this comment.
This error message is not actionable as it doesn't give a hint what to change.
There was a problem hiding this comment.
I have changed to: Invalid address '%s'. If the address contains UTF-8 characters in the local part please ensure the SMTP server supports SMTPUTF8 extensions and enable 'Allow UTF-8' in the email realm configuration.
There was a problem hiding this comment.
Changed to: Invalid sender address '%s'. If the address contains UTF-8 characters in the local part please ensure the SMTP server supports SMTPUTF8 extensions and enable 'Allow UTF-8' in the email realm configuration.
There was a problem hiding this comment.
This is IMHO only half the truth: When not enabling allowuft8, I won't be able to send emails from or to an email address that contains UTF8 characters before the "@". So any user that has entered a domain with such a special character will not be able to receive emails.
There was a problem hiding this comment.
I have just copied the description of the mail.mime.allowutf8 option. What do you prefer prefer here? Say that is only for local part?
There was a problem hiding this comment.
I would bring this into context for a Keycloak admin. For Keycloak, this would only be about addresses, and no other headers.
It would be good to make it more specific about the local parts.
"Enable to allow UTF-8 characters in the local part of the email address. This should only be enabled if the mail server supports UTF-8 via the SMTPUTF8 extension. If disabled, domain names containing UTF-8 characters will be encoded using punycode, and addresses containing UTF-8 characters in the local part of the address will return an error."
There was a problem hiding this comment.
Okis, changed in the documentation and for the admin console help message.
There was a problem hiding this comment.
This is too restrictive, and IMHO points to a different problem: you can have an email address with a domain name that is IDN encoded.
It seems to me that we should pny-encode those domain names before sending them.
Interesting enough the frontend for setting the sender address will encode the email address in pnycode automatically, while a user's email address when fetched from the database when sending the test email will not be pny-encoded.
There was a problem hiding this comment.
Okis, I have changed to use allowutf8 and, if not set, change to the IDN.toASCCI method in the domain part. Note that this was exactly the same before.
There was a problem hiding this comment.
Would it be possible to make this option "true" by default? Is the UTF8 mode supported by the recent versions of the popular SMTP servers?
There was a problem hiding this comment.
I prefer to not do that, at least for now. If we set it to true by default we need to add a task to migrate the realms and add the false (if not, when edited by the admin console it would change to true). We can do in a follow-up only for the main branch. But as commented to @ahus1 we can do it, but I would not backport that part.
There was a problem hiding this comment.
I'm absolutely fine with this approach!
|
@ahus1 I have done some changes but not all of them 😄. In general the
I wouldn't do that, nobody complained about this until now. We can do it, but then I need to add a migration task to add
No, I don't think that it is a notable change. The option is false as before, so we are not changing the behavior.
I added that part. But I'm not testing the email completely, just statically. If I add a full test (test SMTP connection, username/password and so on and so forth), there are tons of tests that fail (because they are importing a realm with email configuration and the SMTP server is not setup).
We could add something similar to that, but I have not done it, mainly because the greenmail we use for testing does not announce SMTPUTF8 (I prefer testing something). The admin should know if they want/support UTF-8 or not. We can check if SMTPUTF8 extension is listed when connecting to send the email, and throw an exception if it is configured and not present. But, as commented, that would break testing. If everybody thinks that it's better, I can add the check when sending an email. |
|
It seems CI fails because we are waiting for #41794. I will rebase tomorrow. |
e669fe8 to
9363b25
Compare
|
I disagree with the following statement:
We know about one email server that is supporting the SMTPUTF8 extension that is throwing an exception. Other email servers might just accept it. Now we are always throwing an exception when there is a local part with UTF-8 characters. So to me this is a breaking change. I vote to remove the check if UTF-8 characters are part of the local part of the email to be backwards compatible. No-one complained that we didn't do that for the past 10 years. |
@ahus1 The angus mail implementation sends the email when the This means that you can do weird things and the reason that this is managed as important issue (not an enhancement), we have a moderate CVE-2025-8419 related to this (UTF-8 characters are transformed without any control and an attacker can inject into the SMTP, not much because the email is very short, but it's moderate). So, the important part of this PR is that we should control the email and reject invalid ones to avoid the issue. |
|
I'm going to add an upgrading note just in case. I don't think that this will affect a lot of people, but it's not the first time I've failed in a prediction like this one. 😄 |
There was a problem hiding this comment.
Nitpick: Maybe validate would be a better name. At least it's more commonly used.
There was a problem hiding this comment.
Yep, you are right. Changed!
There was a problem hiding this comment.
Probably a valid Javadoc would be useful.
There was a problem hiding this comment.
Nitpick: These seem to be separate test cases. Perhaps it would be useful to put them in separate test methods? Maybe we could even execute them in parallel?
There was a problem hiding this comment.
I think that it's better here, we have everything to test email in this class and the class is fast to test (it's not one of the worse ones9.
There was a problem hiding this comment.
This approach works for me as well. Resolving.
|
Ok, I have added the upgrading note and done 2 of 3 of the suggestions from @slaskawi. Let's wait the CI now. I have also squashed the commits into just one. |
|
@rmartinc - I've added one more commit to the docs to give admin who are migrating some hands-on advice. For example, without updating the email validation rule, users might register with an email address that contains non-ASCII characters, and then they wouldn't be able to confirm their email address. I also fixed something that I missed before: While there are ASCII and non-ASCII characters, UTF-8 is an encoding and therefore doesn't have any characters. So I've update the statements. It is getting quite late here, so please re-read those paragraphs. I'll mark my earlier review now resolved, so Marek can eventually proceed. |
|
@ahus1 LGTM and adding another check for emails if you don't support non-ascii in the local part makes lot of sense. I have just changed the expression to use entities and avoid the issue with the variables. |
|
One more comment, note that we can create a new email validator that, if smtp configuration in the realm does not allow UTF-8, checks that the email local part has only ascii characters. This validator can be assigned automatically using User Profile. But I don't know if we are complicating this too much. But just in case. |
Closes keycloak#41023 Signed-off-by: rmartinc <rmartinc@redhat.com>
Signed-off-by: Alexander Schwartz <aschwart@redhat.com>
…detail that admins should not care about. Recommending to enable UTF-8. For the other scenario, describe the steps necessary to prevent having email addresses in the system that can't receive emails from Keycloak Signed-off-by: Alexander Schwartz <aschwart@redhat.com>
Signed-off-by: rmartinc <rmartinc@redhat.com>
|
I'm ok with the change, and just did the rebase. |
Closes #41023
Adding the
allowutf8option to the smtp configuration to configure mail.mime.allowutf8 in the jakarta configuration with it. This allows to send and control emails with UTF-8 characters if the SMTP server supports it. Test added.