Skip to content

hba: stricter parsing and better pretty-printing#43811

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
knz:20200108-strict-hba
Jan 8, 2020
Merged

hba: stricter parsing and better pretty-printing#43811
craig[bot] merged 1 commit intocockroachdb:masterfrom
knz:20200108-strict-hba

Conversation

@knz
Copy link
Copy Markdown
Contributor

@knz knz commented Jan 8, 2020

The following defects were found by @mjibson using
go-fuzz (#43805) and are fixed by this patch:

  • the code would parse then fail to pretty-print an empty connection
    type and auth method.
  • the code would parse then pretty-print incorrectly a connection
    type or auth method containing commas or other special characters.
  • the code would accept an empty string as address.
  • the code would accept then fail to pretty-print columns containing
    special (control) characters.

Note that the code previously supported tabs (\t) and carriage
returns (\r) inside quoted strings, and this is now rejected. For most
fields, this does not matter, as these special characters were not
valid semantically anyway.

However, for the method options in the last column it is theoretically
possible for a method to wish to use such special characters. This is
not currently the case for the CCL gss method already implemented,
so this change is not introducing a regression. A comment is left in
the code to indicate what needs to be changed if a future method needs
this support.

Release note (security): CockroachDB now properly rejects control
characters in the value of the cluster setting
server.host_based_authentication.configuration. These characters
were previously accepted and would silently yield rules matching
differently from intended. Deployments careful to strip control
characters from their HBA configuration are not affected.

@knz knz requested a review from madelynnblue January 8, 2020 14:09
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@knz knz force-pushed the 20200108-strict-hba branch 2 times, most recently from 35dc6f8 to 8bfc6fe Compare January 8, 2020 14:31
The following defects were found by @mjibson using
`go-fuzz` and are fixed by this patch:

- the code would parse then fail to pretty-print an empty connection
  type and auth method.
- the code would parse then pretty-print incorrectly a connection
  type or auth method containing commas or other special characters.
- the code would accept an empty string as address.
- the code would accept then fail to pretty-print columns containing
  special (control) characters.

Note that the code previously supported tabs (\t) and carriage
returns (\r) inside quoted strings, and this is now rejected. For most
fields, this does not matter, as these special characters were not
valid semantically anyway.

However, for the method options in the last column it is theoretically
possible for a method to wish to use such special characters. This is
not currently the case for the CCL `gss` method already implemented,
so this change is not introducing a regression. A comment is left in
the code to indicate what needs to be changed if a future method needs
this support.

Release note (security): CockroachDB now properly rejects control
characters in the value of the cluster setting
`server.host_based_authentication.configuration`. These characters
were previously accepted and would silently yield rules matching
differently from intended. Deployments careful to strip control
characters from their HBA configuration are not affected.
@knz knz force-pushed the 20200108-strict-hba branch from 8bfc6fe to 98231d0 Compare January 8, 2020 15:16
@knz
Copy link
Copy Markdown
Contributor Author

knz commented Jan 8, 2020

thanks!

bors r+

craig bot pushed a commit that referenced this pull request Jan 8, 2020
43792: batcheval: clean up remaning variable names of `{Reader,ReadWriter}` types r=irfansharif a=irfansharif

Ignore first commit, reviewed in #43764. 

We should be using {reader,readWriter} for
{Reader,ReadWriter} respectively instead. I'd skipped everything under
batcheval when working on #43265, back when I thought it'd be a good
idea to plumb in an engine.Batch throughout replica read codepaths. Due
to perf penalty as seen on #43388, I'm no longer going to be doing that.
This is the remaining work.

Release note: None


43811: hba: stricter parsing and better pretty-printing r=knz a=knz

The following defects were found by @mjibson using
`go-fuzz` (#43805) and are fixed by this patch:

- the code would parse then fail to pretty-print an empty connection
  type and auth method.
- the code would parse then pretty-print incorrectly a connection
  type or auth method containing commas or other special characters.
- the code would accept an empty string as address.
- the code would accept then fail to pretty-print columns containing
  special (control) characters.

Note that the code previously supported tabs (\t) and carriage
returns (\r) inside quoted strings, and this is now rejected. For most
fields, this does not matter, as these special characters were not
valid semantically anyway.

However, for the method options in the last column it is theoretically
possible for a method to wish to use such special characters. This is
not currently the case for the CCL `gss` method already implemented,
so this change is not introducing a regression. A comment is left in
the code to indicate what needs to be changed if a future method needs
this support.

Release note (security): CockroachDB now properly rejects control
characters in the value of the cluster setting
`server.host_based_authentication.configuration`. These characters
were previously accepted and would silently yield rules matching
differently from intended. Deployments careful to strip control
characters from their HBA configuration are not affected.

Co-authored-by: irfan sharif <irfanmahmoudsharif@gmail.com>
Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>
@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jan 8, 2020

Build succeeded

@craig craig bot merged commit 98231d0 into cockroachdb:master Jan 8, 2020
@knz knz deleted the 20200108-strict-hba branch January 8, 2020 18:19
@knz
Copy link
Copy Markdown
Contributor Author

knz commented Jan 9, 2020

(found while working on #31113)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants