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

Localize our chroot in/out operations to minimize time spent inside #836

Merged
merged 2 commits into from
Sep 25, 2019

Conversation

pmatilai
Copy link
Member

The primary motivation here is to consolidate all database accesses
on one side of the chroot, currently it happens on both sides of the
border causing all sorts of issues and limitations (such as preventing
us from using more advanced modes of databases).
As a positive side-effect, the sections where we potentially run
inside chroot are more easily identifiable.

Consolidating on the outside may seem counter-productive, to improve
security it seems you'd want to spend as much time in as possible,
including database accesses. Unfortunately due to rpm's access patterns
and API promises, that's not really achievable (tried several approaches,
run into as many dead-ends).

Technically we could localize the chroot placement much further, but
doing so would change the side for transaction callbacks, which could
cause nasty breakage for our API users as various clients use those
callback slots to update their own databases and logs. So the chroot
spots here are selected to cover minimum possible code while preserving
the chroot side of callbacks and plugin slots: RPMCALLBACK_INST_OPEN/CLOSE,
ELEM_PROGRESS and VERIFY_* occur outside the chroot, everything else inside.
Of plugin slots, init/cleanup and tsm_pre/post occur outside, everything
else inside.

@pmatilai pmatilai force-pushed the dbtrans-pr branch 2 times, most recently from 5184018 to 746c9e3 Compare September 13, 2019 11:56
@Conan-Kudo
Copy link
Member

@pmatilai Is this something you want to also include for rpm 4.15?

@pmatilai
Copy link
Member Author

pmatilai commented Sep 18, 2019

Heavens forbid, no. Not at this point in the release process.

@pmatilai
Copy link
Member Author

Hmph, there's an elephant or two in the room. Of course there is...

There are two major cases unresolved by this: fingerprinting, and rpm -V verification. Fixing verify is merely annoying, but fingerprinting is much harder and more intrusive as there are several far-apart places where we stat() stuff.

However those are always read-only accesses, which seems to be mostly harmless and nothing seems to mind. The cases where it matters are open, close and writing.

So at the very least, the commit message needs to clarify the chroot rules being laid out here, and that's what this is largely about: while rpm itself can generally be fixed to function on whichever side of chroot, that's not the case for external libraries we might call, and there's also our own API users and their external libraries that can be affected by all this.

rpmpsmRun() has gathered so much extra logic around the beef that this
seems reasonable. In particular this makes the next commit much nicer.
The primary motivation here is to consolidate all database writes
(open, write, close) on one side of the chroot, currently it happens on
both sides of the border causing all sorts of issues and limitations (such
as preventing more advanced modes of BDB, not to mention other databases).
As a positive side-effect, the sections where we potentially run
inside chroot are more easily identifiable.

Consolidating on the outside may seem counter-productive, to improve
security it seems you'd want to spend as much time *in* as possible,
including database accesses. Unfortunately due to rpm's access patterns
and API promises, that's not really achievable (tried several approaches,
run into as many dead-ends).

Technically we could localize the chroot placement much further, but
doing so would change the side for transaction callbacks, which could
cause nasty breakage for our API users as various clients use those
callback slots to update their own databases and logs. So the chroot
spots here are selected to cover minimum possible code while preserving
the chroot side of callbacks and plugin slots: RPMCALLBACK_INST_OPEN/CLOSE,
ELEM_PROGRESS and VERIFY_* occur outside the chroot, everything else inside.
Of plugin slots, init/cleanup and tsm_pre/post occur outside, everything
else inside.
@pmatilai
Copy link
Member Author

Refactored into two commits to eliminate the indentation problem in the first version, and clarified commit message.

@pmatilai
Copy link
Member Author

Since there are no further comments...

@pmatilai pmatilai merged commit 0508e9a into rpm-software-management:master Sep 25, 2019
@mlschroe
Copy link
Contributor

My only concern is that the password/group lookups in rpmug.c need to be done in the chroot, but you probably already checked that.

@pmatilai pmatilai deleted the dbtrans-pr branch October 2, 2019 11:28
pmatilai added a commit to pmatilai/rpm that referenced this pull request Oct 16, 2019
All normal functionality is expected to work. Automatic generation
of missing index tables is missing, but that's not relevant at this time.
Going forward, we'll also want some sort of compatibility tracking
for the sql schema.

The database scheme basically just mirrors what BDB does, using strings
for strings and blobs for everything else due to the way integers are
handled in the sqlite C API, for now at least.

Performance is similar or better with BDB in the current unsafe CDB
model, but sqlite uses proper database transactions so this is expected
to be an order of magnitude more robust.

Many things are stupid and/or kind of backwards here due to the internal
API, which I've avoided changing in order to keep it backportable for the
time being. rpm-software-management#836 is
needed but otherwise this should drop quite trivially into 4.14.x too.
However as we're planning for a longer term future here, it would be dumb
to limit ourselves by what's possible with an internal BDB-oriented API,
so I've fairly major changes planned in that direction.

Note that this changes the default backend for the sake of testing
it in the CI, that will need to be removed before merging, the time to
talk about default changes is (much) later.
pmatilai added a commit to pmatilai/rpm that referenced this pull request Oct 17, 2019
All normal functionality is expected to work. Automatic generation
of missing index tables is missing, but that's not relevant at this time.
Going forward, we'll also want some sort of compatibility tracking
for the sql schema.

The database scheme basically just mirrors what BDB does, using strings
for strings and blobs for everything else due to the way integers are
handled in the sqlite C API, for now at least. Some amount of schema
changes are to be expected before this is considered final.

Performance is similar or better with BDB in the current unsafe CDB
model, but sqlite uses proper database transactions so this is expected
to be an order of magnitude more robust.

Many things are stupid and/or kind of backwards here due to the internal
API, which I've avoided changing in order to keep it backportable for the
time being. rpm-software-management#836 is
needed but otherwise this should drop quite trivially into 4.14.x too.
However as we're planning for a longer term future here, it would be dumb
to limit ourselves by what's possible with an internal BDB-oriented API,
so I've fairly major changes planned in that direction.
pmatilai added a commit that referenced this pull request Oct 18, 2019
All normal functionality is expected to work. Automatic generation
of missing index tables is missing, but that's not relevant at this time.
Going forward, we'll also want some sort of compatibility tracking
for the sql schema.

The database scheme basically just mirrors what BDB does, using strings
for strings and blobs for everything else due to the way integers are
handled in the sqlite C API, for now at least. Some amount of schema
changes are to be expected before this is considered final.

Performance is similar or better with BDB in the current unsafe CDB
model, but sqlite uses proper database transactions so this is expected
to be an order of magnitude more robust.

Many things are stupid and/or kind of backwards here due to the internal
API, which I've avoided changing in order to keep it backportable for the
time being. #836 is
needed but otherwise this should drop quite trivially into 4.14.x too.
However as we're planning for a longer term future here, it would be dumb
to limit ourselves by what's possible with an internal BDB-oriented API,
so I've fairly major changes planned in that direction.
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.

None yet

3 participants