Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Audit Logs for Postgres connections opened through a data link #9873

Merged
merged 58 commits into from
May 11, 2024

Conversation

radeusgd
Copy link
Member

@radeusgd radeusgd commented May 7, 2024

Pull Request Description

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    TypeScript,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • Unit tests have been written where possible.

Comment on lines +93 to +96
@Override
public DatabaseMetaData getMetaData() throws SQLException {
return underlying.getMetaData();
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently metadata is not audited. That means that the act of listing tables existing in the database is not audited.

Do you think we should change that? It was unclear to me if it's something that we want to log or not. It is easy to add if we decided that we do want it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree don't think we need to audit Metadata queries.

@radeusgd
Copy link
Member Author

radeusgd commented May 8, 2024

I was curious what happens if I open an SQLite database file stored in the Enso Cloud (or S3).

It was failing with Method read_stream of type SQLite_Format could not be found..

So I added the missing method, changing the failure to a clearer: Illegal_Argument.Error 'Cannot connect to a SQLite database backed by a stream. Save it to a local file first.'

Copy link
Member

@jdunkerley jdunkerley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great - a few little questions.

Comment on lines +93 to +96
@Override
public DatabaseMetaData getMetaData() throws SQLException {
return underlying.getMetaData();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree don't think we need to audit Metadata queries.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come this needs to be in Standard.Base?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that Enso_Cloud.Internal.Utils used here are private.

We'd need to start running Base_Tests with --disable-private-check. But tbh I'm worried about doing that as I already noted somewhere. If we run our whole test suite with --disable-private-check then we are testing with different settings than used in production. We already ran into problems where private keyword introduced regressions - the display method that I fix in this PR got broken because of it. Here the regression went unnoticed because our tests did not cover the display method. But if we then --disable-private-check for our test suite, we can have such undetected regressions in other places, only to be seen when used in production environments that lack the --disable-private-check parameter.

On the other hand I remember we wanted to restructure our tests in such a way that they'd be able to access private methods of the tested package by default. Given that Enso is an inherently dynamic language, the problems described in the paragraph above convince me this may not be the safest thing to do.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still, I agree that these utils probably should not be part of the library.

I can move them to the tests and change so that Base_Tests are run with --disable-private-check for now. But I think for longer term some better solution is needed.

My proposal (that's just one way we could do that) would be to:

  • change --disable-private-check to --allow-disabling-private-check and changing the methods of overcoming private acccess to be more localized:
  • for the static part - i.e. imports - introduce a private import A.B.C syntax that allows to import private modules,
  • for the dynamic part, introduce Context.Private_Access.with_enabled that allows to enable private access only within a delimited scope.

The two new constructs would only be allowed if --allow-disabling-private-check flag was passed to the interpreter. If no such flag was passed, the private import would need to result in a compiler error, and the Context.Private_Access.with_enabled would panic. Thus in normal usage of Enso, private access will not be allowed, and within our tests (that would have this flag turned on), we can enable the access but only if explicitly opted-in, delimited to the minimal scopes where this is really needed (e.g. only to implement such Test_Utils helper). Thus all other tests that do not rely on exposing privacy would be unaffected and still test the behaviour as it is working in production configurations.

@radeusgd
Copy link
Member Author

radeusgd commented May 9, 2024

image

Report from OpenSearch logs, sorted by the sequence number. I highlighted a block corresponding to a table update rows transaction.

@radeusgd radeusgd added the CI: Ready to merge This PR is eligible for automatic merge label May 9, 2024
@mergify mergify bot merged commit 5f0a16c into develop May 11, 2024
34 checks passed
@mergify mergify bot deleted the wip/radeusgd/9599-audited-postgres branch May 11, 2024 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Created audit logged JDBC connection for Postgres based Datalinks
3 participants