Issue 9986

Title
Make URL is_secure when SSL is delegated
Priority
bug
Status
resolved
Nosy list
ced, pokoli, reviewbot, roundup-bot, yangoon
Assigned to
ced
Keywords
review

Created on 2021-01-10.23:59:07 by ced, last changed 9 months ago by roundup-bot.

Messages

New changeset 049bdc054e69 by Cédric Krier in branch '5.8':
Support secure URL when SSL is delegated
https://hg.tryton.org/trytond/rev/049bdc054e69

New changeset 5d84bdc3e115 by Cédric Krier in branch '5.6':
Support secure URL when SSL is delegated
https://hg.tryton.org/trytond/rev/5d84bdc3e115
New changeset 803edf1fb057 by Cédric Krier in branch 'default':
Support secure URL when SSL is delegated
https://hg.tryton.org/tryton-env/rev/803edf1fb057
New changeset 6455d1dbf4d5 by Cédric Krier in branch 'default':
Support secure URL when SSL is delegated
https://hg.tryton.org/trytond/rev/6455d1dbf4d5
Author: [hidden] (pokoli) Tryton committer Tryton translator
Date: 2021-01-22.11:09:48

El 22/1/21 a les 11:02, Cédric Krier ha escrit:

If there is no case I think we can raise and error if delegated is set and any of private_key/certificate is also set to make the user aware of wrong configuration values.
That is what I do not want, having to validate the configuration.

Why not? We already do it for example when wrong price_digits are set.

Indeed I prefer to have an error message when I did something wrong that spend 2 hours trying to find by myself with something is not working.

Using only two parameters may lead to a wrong configuration without notice for the user: setting the private_key but not the certificate.
User writing wrong configuration can always happen.

Of course. The problem is that writing wrong configuration without noticing.

Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2021-01-22.11:02:03
On 2021-01-22 10:49, Sergi Almacellas Abellana wrote:
> 
> Sergi Almacellas Abellana <sergi@koolpi.com> added the comment:
> 
> El 22/1/21 a les 10:40, Cédric Krier ha escrit:
> >> When activating the delegated option the privatekey and certificate is ignored.
> > That's your interpretation. That's why I said it is ambiguous. You have configuration that does not mean anything depending on other.
> 
> If the code is in this way and the documentation clearly said that its the behaviour I do not think anyone should interpet it in another way.

So your counterproposal does not improve the situation. It still
requires to document special cases. But it must document many special
cases.
This is because we need only 3 states which are completely covered by 2
variables. But using 3 variables add more states that have no meaning.

> If there is no case I think we can raise and error if delegated is set and any of private_key/certificate is also set to make the user aware of wrong configuration values.

That is what I do not want, having to validate the configuration.

> Using only two parameters  may lead to a wrong configuration without notice for the user:  setting the private_key but not the certificate.

User writing wrong configuration can always happen.
Author: [hidden] (yangoon) Tryton translator
Date: 2021-01-22.10:51:03

When activating the delegated option the privatekey and certificate is ignored.

I would handle it exactly the other way:

When SSL is activated secure URLs are returned anyway and setting ssl_delegated will have no meaning.

The examples

ssl_delegated=False
privatekey=/path/key
certificate=/path/cert

will activate SSL and secure URLs will be returned

ssl_delegated=True
privatekey=/path/key
certificate=

will return secure URLs, because SSL is not activated.

That is just the unambigous behavior as written in the Notes of my proposal.

Author: [hidden] (pokoli) Tryton committer Tryton translator
Date: 2021-01-22.10:49:04

El 22/1/21 a les 10:40, Cédric Krier ha escrit:

When activating the delegated option the privatekey and certificate is ignored.
That's your interpretation. That's why I said it is ambiguous. You have configuration that does not mean anything depending on other.

If the code is in this way and the documentation clearly said that its the behaviour I do not think anyone should interpet it in another way.

Do you have any case where the delegated should be set to true and the private/public key should be used?

If there is no case I think we can raise and error if delegated is set and any of private_key/certificate is also set to make the user aware of wrong configuration values. Using only two parameters may lead to a wrong configuration without notice for the user: setting the private_key but not the certificate.

Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2021-01-22.10:40:35

When activating the delegated option the privatekey and certificate is ignored.

That's your interpretation. That's why I said it is ambiguous. You have configuration that does not mean anything depending on other.

Author: [hidden] (pokoli) Tryton committer Tryton translator
Date: 2021-01-22.10:33:57

The first one means that tryton is serving the HTTPS from the privatekey and certificate path (delegated is False).
The second one means tryton should not serve ssl as it's delegated.

When activating the delegated option the privatekey and certificate is ignored.

Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2021-01-22.10:21:19

This is an ambiguous syntax. What means:

ssl_delegated=False
privatekey=/path/key
certificate=/path/cert

or

ssl_delegated=True
privatekey=/path/key
certificate=
Author: [hidden] (pokoli) Tryton committer Tryton translator
Date: 2021-01-22.09:54:33

>
>

My proposal:

section ssl

ssl_delegated
Return secure URLs

Note: Activate this setting if ssl support is delegated to a proxy. Tryton will return secure URLs without doing SSL on its own.

privatekey
The path to the private key.

certficate
The certificate for the private key.

Note: To activate SSL both privatekey and certificate must be set.

I like your proposal just some sugestions:

  1. I will rename ssl_delegated to delegated

  2. We need to make clear that privatekey and certificate will be ignored if delegated is set. So probably better to document delegated after privatekey and certificate and include a note for it.

Author: [hidden] (yangoon) Tryton translator
Date: 2021-01-22.09:48:33

Introduced a third variable creates situation where configuration is not valid and has no meaning.

Could you expand on this?

My proposal:

section ssl

ssl_delegated
Return secure URLs

 Note: Activate this setting if ssl support is delegated to a proxy. Tryton will return secure URLs without doing SSL on its own.

privatekey
The path to the private key.

certficate
The certificate for the private key.

Note: To activate SSL both  privatekey and certificate must be set.
Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2021-01-22.01:14:19

Introduced a third variable creates situation where configuration is not valid and has no meaning.

Author: [hidden] (yangoon) Tryton translator
Date: 2021-01-15.11:29:45

Ping. Any comment on this?

Author: [hidden] (yangoon) Tryton translator
Date: 2021-01-11.00:20:28

I think that the ssl configuration is even more obscured by this change. This feature deserves without doubt a dedicated setting. When the change is backported it is nevertheless an API breakage. So I don't see the difference between the proposed change and the introduction of a new dedicated and meaningful setting.

Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2021-01-10.23:59:07

A common issue is that when the SSL is delegated to a proxy server, the URL built without a requests are considered as non secure. This is because we rely on the presence of a certificate/privatekey in the configuration.
This can be seen in the email sent when the admin password is reset via command line.
I propose to allow to set only one of the two configuration keys so the internal web server will not try to setup SSL and make is_secure return True.
This proposal has minor impact and so it can be back-ported without too much pain.

History
Date User Action Args
2021-02-24 21:25:56roundup-botsetkeyword: - backport
messages: + msg64843
2021-02-15 19:11:21roundup-botsetmessages: + msg64616
2021-02-15 19:11:17roundup-botsetmessages: + msg64615
nosy: + roundup-bot
status: testing -> resolved
2021-02-09 01:10:34reviewbotsetmessages: + msg64409
2021-01-22 11:42:18reviewbotsetmessages: + msg64042
2021-01-22 11:09:48pokolisetmessages: + msg64041
2021-01-22 11:02:03cedsetmessages: + msg64040
2021-01-22 10:51:03yangoonsetmessages: + msg64039
2021-01-22 10:49:04pokolisetmessages: + msg64038
2021-01-22 10:40:35cedsetmessages: + msg64037

Showing 10 items. Show all history (warning: this could be VERY long)