Обсуждение: Incorrect comment in be-secure-openssl.c

The comment in be-secure-openssl.c didn't get the memo that the hardcoded DH
parameters moved in 573bd08b99e277026e87bb55ae69c489fab321b8.  The attached
updates the wording, keeping it generic enough that we wont need to update it
should the parameters move again.

cheers ./daniel

On Thu, May 28, 2020 at 05:15:17PM +0200, Daniel Gustafsson wrote:
> The comment in be-secure-openssl.c didn't get the memo that the hardcoded DH
> parameters moved in 573bd08b99e277026e87bb55ae69c489fab321b8.  The attached
> updates the wording, keeping it generic enough that we wont need to update it
> should the parameters move again.

Indeed, looks good to me.  I'll go fix, ust let's wait and see first
if others have any comments.
--
Michael

On Fri, May 29, 2020 at 02:38:53PM +0900, Michael Paquier wrote:
> Indeed, looks good to me.  I'll go fix, ust let's wait and see first
> if others have any comments.

Actually, I was reading again the new sentence, and did not like its
first part.  Here is a rework that looks much better to me:
  * Load hardcoded DH parameters.
  *
- * To prevent problems if the DH parameter files don't even exist, we
- * can load hardcoded DH parameters supplied with the backend.
+ * If DH parameters cannot be loaded from a specified file, we can load
+ * the hardcoded DH parameters supplied with the backend to prevent
+ * problems.

Daniel, is that fine for you?
--
Michael

On Sun, May 31, 2020 at 2:54 AM Michael Paquier <michael@paquier.xyz> wrote:
> On Fri, May 29, 2020 at 02:38:53PM +0900, Michael Paquier wrote:
> > Indeed, looks good to me.  I'll go fix, ust let's wait and see first
> > if others have any comments.
>
> Actually, I was reading again the new sentence, and did not like its
> first part.  Here is a rework that looks much better to me:
>   * Load hardcoded DH parameters.
>   *
> - * To prevent problems if the DH parameter files don't even exist, we
> - * can load hardcoded DH parameters supplied with the backend.
> + * If DH parameters cannot be loaded from a specified file, we can load
> + * the hardcoded DH parameters supplied with the backend to prevent
> + * problems.
>
> Daniel, is that fine for you?

I don't understand why that change is an improvement.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Sun, May 31, 2020 at 05:47:01PM -0400, Robert Haas wrote:
> On Sun, May 31, 2020 at 2:54 AM Michael Paquier <michael@paquier.xyz> wrote:
> I don't understand why that change is an improvement.

Oops.  I have managed to copy-paste an incorrect diff.  The existing
comment is that:
 * To prevent problems if the DH parameters files don't even
 * exist, we can load DH parameters hardcoded into this file.

Daniel's suggestion is that:
 * To prevent problems if the DH parameters files don't even
 * exist, we can load hardcoded DH parameters supplied with the backend.

And my own suggestion became that:
 * If DH parameters cannot be loaded from a specified file, we can load
 * the hardcoded DH parameters supplied with the backend to prevent
 * problems.

The problem I have with first and second flavors is that "DH
parameters files" does not sound right.  First, the grammar sounds
incorrect to me as in this case "parameters" should not be plural.
Second, it is only possible to load one file with ssl_dh_params_file,
and we only attempt to load this single file within initialize_dh().

Of course it would be possible to just switch to "DH parameter file"
in the first part of the sentence, but I have just finished by
rewriting the whole thing, as the third flavor.
--
Michael

> On 1 Jun 2020, at 08:06, Michael Paquier <michael@paquier.xyz> wrote:

> The problem I have with first and second flavors is that "DH
> parameters files" does not sound right.  First, the grammar sounds
> incorrect to me as in this case "parameters" should not be plural.

I think "parameters" is the right term here, as the shared secret is determines
a set of Diffie-Hellman parameters.

> Second, it is only possible to load one file with ssl_dh_params_file,
> and we only attempt to load this single file within initialize_dh().

Thats correct, this is a leftover from when we allowed for different DH sizes
and loaded the appropriate file.  This was removed in c0a15e07cd718cb6e455e683
in favor of only using 2048.

> Of course it would be possible to just switch to "DH parameter file"
> in the first part of the sentence, but I have just finished by
> rewriting the whole thing, as the third flavor.

I don't have a problem with the existing wording of the first sentence, and I
don't have a problem with your suggestion either (as long as it's parameters in
plural).

cheers ./daniel

On Mon, Jun 01, 2020 at 10:39:45AM +0200, Daniel Gustafsson wrote:
> I don't have a problem with the existing wording of the first sentence, and I
> don't have a problem with your suggestion either (as long as it's parameters in
> plural).

Thanks, that's why I kept the word plural in my own suggestion.  I was
just reading through the whole set again, and still kind of prefer the
last flavor, so I think that I'll just fix it this way tomorrow and
call it a day.
--
Michael

> On 3 Jun 2020, at 14:38, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Mon, Jun 01, 2020 at 10:39:45AM +0200, Daniel Gustafsson wrote:
>> I don't have a problem with the existing wording of the first sentence, and I
>> don't have a problem with your suggestion either (as long as it's parameters in
>> plural).
>
> Thanks, that's why I kept the word plural in my own suggestion.  I was
> just reading through the whole set again, and still kind of prefer the
> last flavor, so I think that I'll just fix it this way tomorrow and
> call it a day.

Sounds good, thanks!

cheers ./daniel

On Wed, Jun 03, 2020 at 02:40:54PM +0200, Daniel Gustafsson wrote:
> Sounds good, thanks!

Okay, done then.
--
Michael