Hello
Originally I started looking at this thread because Jacob mentioned it
relates to the custom OAuth / HBA variable question[1]. While I do see
the relation, I don't have many practical ideas.
I was thinking about suggesting using a "key=value, key=value ..."
file style instead of the fixed table, both for easier later
generalization, and because it aligns better to modern configuration
formats (and personally I always find these columnar config files
harder to read)
However, it would also differ from other existing postgres config
files and wouldn't offer a clear initial advantage, so it doesn't seem
like a good practical choice. I'm still mentioning this for
completeness, but mostly I'll focus on a more practical review:
+ check_nook => 'check_ssl_sni',
This seems to be a typo?
+ if (SSL_client_hello_get0_ext(ssl, TLSEXT_TYPE_server_name, &tlsext, &left))
+ {
+ if (left <= 2)
+ {
+ *al = SSL_AD_MISSING_EXTENSION;
+ return 0;
+ }
... and later error returns in this if block seem to use the wrong
error code to me: truncated length, length mismatch, empty list,
length exceeding remaining data...
missing_extension: Sent by endpoints that receive a handshake
message not containing an extension that is mandatory to send for
the offered TLS version or other negotiated parameters.
decode_error: A message could not be decoded because some field was
out of the specified range or the length of the message was
incorrect. This alert is used for errors where the message does
not conform to the formal protocol syntax. This alert should
never be observed in communication between proper implementations,
except when messages were corrupted in the network.
Since we are already inside the if which verifies that the extension
is present, shouldn't all of these report decode_error?
+ if (!ssl_update_ssl(ssl, install_config))
+ {
+ ereport(COMMERROR,
+ errcode(ERRCODE_PROTOCOL_VIOLATION),
+ errmsg("failed to switch to SSL configuration for host, terminating
connection"));
+ return SSL_CLIENT_HELLO_ERROR;
+ }
Isn't there a missing *al = assignment here?
+ /*
+ * There should be no more tokens after this, if there are break
+ * parsing and report error to avoid silently accepting incorrect
+ * config.
+ */
+ if (tokens->length > 1)
+ {
+ ereport(elevel,
+ errcode(ERRCODE_CONFIG_FILE_ERROR),
+ errmsg("extra fields at end of line"),
+ errcontext("line %d of configuration file \"%s\"",
+ tok_line->line_num, tok_line->file_name));
+ return NULL;
+ }
The comment suggests that this aims to prevent any additional text on
the line, but this parses:
localhost server.crt server.key server.crt "cmd" on TRAILING_TEXT MORE_TEXT
+ /* SSL Passphrase Command (optional) */
+ field = lnext(tok_line->fields, field);
+ if (field)
+ {
+ tokens = lfirst(field);
+ token = linitial(tokens);
Isn't a length > 1 error check missing from here?
[1]:
https://www.postgresql.org/message-id/CAN4CZFM3b8u5uNNNsY6XCya257u%2BDofms3su9f11iMCxvCacag%40mail.gmail.com
