Ticket #175 (closed task: fixed)

Opened 5 years ago

Last modified 4 years ago

Exception to link against bundled copy of crypsetup in volume_key

Reported by: s4504kr Owned by: jstanley
Priority: major Keywords:
Cc: mitr, toshio, agk, mbroz Blocked By:
Blocking:

Description

on a review fo volume-key

https://bugzilla.redhat.com/show_bug.cgi?id=508954

I have find out, that the maintainer want to link agains a static release of cryptsetup. this static version of cryptsetup is patched by the maintainer in a way, so new interfaches may be added to this libraries.

He told me, that he don't want to patch the common cryptsetup library until upstream has make the inclussion of this patch.

Change History

comment:1 Changed 5 years ago by toshio

  • Summary changed from Exception to link agains static release of crypsetup in volume_key to Exception to link against bundled copy of crypsetup in volume_key

This is not a request to link against a static library. It's a request to link against a bundled library. Changing subject.

comment:2 Changed 5 years ago by toshio

In terms of packages that would be relatively risk-free to bundle libraries, this doesn't seem to fit the bill at all. If I understand it, this program is going to be run as root and is expected to read and write keys used to encrypt volumes. Security flaws and bugs in the included cryptsetup has a high risk.

Many of the reasons on https://fedoraproject.org/wiki/No_Bundled_Libraries do not apply to this case because it is a port to a newer version of the library instead of bundling an older version however, that's not in itself a reason for allowing an exception. Each of the reasons on that page is a serious reason for disallowing bundling.

The ones that do apply would be all of the security and bugfix reasons. Some of the forking arguments if the patch has not been accepted upstream (I'm unclear on that -- there's mention in the review bug that "The upstream maintainer has accepted the proposed functionality" but also "...other users might mistakenly think these interfaces were accepted upstream" and "Library extensions are planned to be included in uspstream cryptsetup but not in current 1.0.x stable branch (I expect it in cryptsetup 1.1, but discussion still ongoing)."

Another option would be to have a cryptsetup1.1 package that ships a snapshot of the library in a private directory. Note that some of the dangers still remain in this setup -- is the person in charge of the cryptsetup1.1 package watching upstream closely? Are they able to get updates to new snapshots out quickly in case of security issues? Will volume-key block updates of new cryptsetup1.1 packages with security flaws or will volume-key be properly broken until it can catch up in those cases?

comment:3 Changed 5 years ago by kkofler

I'd tend towards approving any "bundled library" which is significantly forked (as seems to be the case here). Upstream obviously had their reasons to fork the library and it's really nontrivial to make it use the system version due to the significant changes, so trying to insist on system libraries would just prevent shipping the application in Fedora at all, which I don't think is the goal of the policy. Requiring to use the system version only makes sense if the system version is actually equivalent to the code which is being used.

But of course this is just my personal opinion. Please DO NOT APPROVE this package before FESCo made a decision.

comment:4 Changed 5 years ago by toshio

The situation you describe is better handled by forking than by bundling. Even then, there are drawbacks to forking over trying to work with upstream to get your changes merged. In this particular case, the library in question may have even accepted the changes upstream (needs clarification from the packagers in question) and it hasn't been released in a tarball yet.

comment:5 follow-up: ↓ 8 Changed 5 years ago by mitr

  • Cc mitr added

(I'm the author of the volume_key package.)

IMHO there is almost no additional security risk: the code is run as root, and the data it manipulates can only be modified by root (or someone who can write to raw disks, which is almost equivalent). The threats against both the volume_key application and libcryptsetup when used by volume_key are thus very low - e.g. much lower than e.g. threats against a picture viewer that can open data downloaded from the internet.

As for upstream status - upstream agrees with the goals of the patch, but it was not reviewed yet and a change to the interface is possible.

Finally, I could have done this "cleanly" by copying parts of libcryptsetup into volume_key, adapting it for the specific use case of volume_key, and not using the "external" libcryptsetup at all. That would have easily passed the review, but it's unquestionably a worse solution than relying on modified libcryptsetup with a clear plan to use external libcryptsetup in a few weeks or months.

comment:6 Changed 5 years ago by kkofler

mitr's explanation makes sense to me. I propose we approve this, with the understanding that it's a temporary solution and efforts should be made to get the changes upstreamed into libcryptsetup ASAP. As (AIUI) there won't be a meeting this week, should we table this for next week's meeting? Or can we vote on this on the bug tracker or something? (I'm +1 to my proposal, obviously.)

comment:7 Changed 5 years ago by toshio

  • Cc toshio added

mitr's explanation makes sense to me.

mitr's explanation makes sense but ignores half of the assessment. It addresses the fact that you can better control the inputs to the program when the data being manipulated is only modifiable by root. However, it ignores the fact that any code run by root has the potential to do greater harm as well. Let's say that the libcryptsetup that is being used in volume_key have a bug that corrupts the headers of encrypted disks in some scenarios. That wouldn't be possible from an imageviewer that runs as a normal user.

Problems like these are serious issues. They can suffer from the same problems with QA and time lag as a security bug. IE: fixes in the package that mirrors upstream will take time to propogate to the copies in other packages.

comment:8 in reply to: ↑ 5 ; follow-up: ↓ 9 Changed 5 years ago by toshio

As for upstream status - upstream agrees with the goals of the patch, but it was not reviewed yet and a change to the interface is possible.

Okay. So this is like any other patch that we've proposed but not gotten into upstream yet.

Finally, I could have done this "cleanly" by copying parts of libcryptsetup into volume_key, adapting it for the specific use case of volume_key, and not using the "external" libcryptsetup at all. That would have easily passed the review, but it's unquestionably a worse solution than relying on modified libcryptsetup with a clear plan to use external libcryptsetup in a few weeks or months.

This line of reasoning is full of failure.

  1. If it was discovered that the code for cryptsetup is being used with some minor changes, then it would still fall under this Guideline.
  2. Would you really create worse software with more work for you to embed the code and then later disentangle the code?
  3. If it is going to land in a few weeks or months, then it shouldn't be hard to convince mbroz to land the patches in cryptsetup in rawhide.
  4. Since upstream has not reviewed the patch yet, we don't know when it will land so the estimate of weeks or months is not based on facts.
  5. The real way to resolve this in the face of the main cryptsetup package sticking with the stable release is to make a new cryptsetup package with the patches you need.

comment:9 in reply to: ↑ 8 Changed 5 years ago by mitr

Replying to toshio:

Finally, I could have done this "cleanly" by copying parts of libcryptsetup into volume_key, adapting it for the specific use case of volume_key, and not using the "external" libcryptsetup at all. That would have easily passed the review, but it's unquestionably a worse solution than relying on modified libcryptsetup with a clear plan to use external libcryptsetup in a few weeks or months.

This line of reasoning is full of failure.

  1. If it was discovered that the code for cryptsetup is being used with some minor changes, then it would still fall under this Guideline.

(If I did that, I would have done the integration thoroughly, e.g. changing the interfaces a lot more, to make the code consistent with the rest of volume_key - but that's beside the point.)

  1. Would you really create worse software with more work for you to embed the code and then later disentangle the code?

The existence of volume_key blocks testing of anaconda builds and patches that use it. If I had to choose between waiting one or two months, with very high risk of missing F12, and spending a week to create a custom LUKS implementation (and those were the only options), I'd create the custom implementation.

  1. If it is going to land in a few weeks or months, then it shouldn't be hard to convince mbroz to land the patches in cryptsetup in rawhide.

I don't think I can ask him to commit to the interfaces before he reviews the patch.

  1. Since upstream has not reviewed the patch yet, we don't know when it will land so the estimate of weeks or months is not based on facts.

True. On the other hand, I believe I have made my intent and commitment to remove the static copy as soon as possible clear.

  1. The real way to resolve this in the face of the main cryptsetup package sticking with the stable release is to make a new cryptsetup package with the patches you need.

OK, if that's what FESCO wants - using a redhat_* namespace for the interfaces.

comment:10 Changed 5 years ago by kkofler

@toshio: The purpose of the guideline against bundled libs is to prevent all those dozens of useless verbatim or almost verbatim copies of zlib and the like. Applying it to modified code from a library is overstretching it, and IMHO goes against both the letter and the spirit of the guideline. Using a system library only makes sense where such a library exists and provides the required functionality. I really don't like code duplication, but sometimes there's no other way. And in this case, upstream is a Fedora contributor and knows very well that this needs to be fixed as soon as possible. Blocking the software until upstream libcryptsetup gets the required functionality would be really unhelpful.

comment:11 Changed 5 years ago by toshio

@kkofler: Considering that I was present for the writing of the guideline and have been one of the people working on the clarifications to it since then, I think you have a better chance of arguing that the guideline is wrong then that the spirit of the guideline is being violated by what I'm arguing for. Modified versions of libraries bundled in applications is definitely covered by the Guideline. Java packages in particular, have had to make modifications for this in the past. Code duplication, which implies it's a simple matter of keeping the size of the distro down, is the least important of the problems with bundling libraries (so unimportant, that I don't even mention size on the No_Bundled_Libraries page.

And BTW I'm not arguing that the volume_key software be blocked, I'm arguing that including a bundled library be blocked. I've pointed out that there's a legitimate method of unbundling while going forward with packaging volume_key -- put the forked library in a separate package, review it, and make volume_key depend on that version.

comment:12 Changed 5 years ago by jstanley

  • Keywords meeting added

comment:13 Changed 5 years ago by agk

  • Cc agk added

comment:14 Changed 5 years ago by mbroz

  • Cc mbroz added

comment:15 follow-up: ↓ 16 Changed 5 years ago by mbroz

The whole discussion is missing basic facts:

  • volume-key requires that LUKS library can use provided (pre-generated) Master Key (currently Master Key is always generated internally)
  • this functionality is not in upstream library, mitr wrote patch for library
  • Requested patch is not distro specific, it adds significant functionality to library
  • on-disk LUKS format is unchanged, everything generated by volume-key is compatible with unpatched cryptsetup/LUKS
  • inclusion upstream (saying that as one of upstream maintainers) require at least
    • proper review
    • expose this functionality to user through cryptsetup command line too (add some extension to format device with provided master key)
    • increase library version

Currently is cryptsetup svn trunk open only for critical patches for 1.0.7 stable release.

There are severeal other patches waiting for next library release, this patch is on TODO list and I see no real problems with inclusion of it in next version. There is no ETA yet.

Resume:

Both cryptsetup and volume-key maintainers agree that ideal temporary solution is statically link volume-key to patched library (copy of upstream in volume-key source). This allows testing of volume-key without possibly introducing problems or compatibility issues to other code or packages.

Cryptsetup maintainer is aware of the code duplication and will notify volume-key maintainer when new version of library is available and volume-key requires rebuild to this library.

p.s.

Posible forking of crypsetup library and exposing fork to whole distro is IMHO the worst solution possible, destroying my upstream effort to stabilize the cryptsetup source code.

Fedora will receive the updated code immediately when available.

comment:16 in reply to: ↑ 15 Changed 5 years ago by jstanley

Replying to mbroz:

Currently is cryptsetup svn trunk open only for critical patches for 1.0.7 stable release.

Cool, our concern was that if the API changed between now and what was committed upstream, then we'd be in a position of having busted code when using the system library, and never get rid of this bundled copy.

If you can tell us that the patch as of now looks fine, and only minor implementation details are likely to be changed, if anything, then that's great.

comment:17 Changed 5 years ago by toshio

I would like to know if we are planning to ship volume_key compiled against a private cryptsetup in F12. Is this a temporary workaround to get the package approved and building in RawHide? so it can be tested for F12 or are we going to be stuck with both cryptsetup and the forked copy in volume_key for the length of F12?

comment:18 Changed 5 years ago by mitr

I want to drop the bundled copy in as soon as possible, both in rawhide and F12. I can't see why it could be desirable to keep it bundled longer.

comment:19 follow-up: ↓ 20 Changed 5 years ago by toshio

Right... but dropping the bundle depends on having cryptsetup with the additional functionality available. I was reading mbroz's "Increase library version" as increasing the SONAME (which would be incompatible and therefore not a good candidate for pushing to F12 after it is released). But reading it again, I'm guessing that he really means, "this goes into 1.0.8 (or 1.1.0), rather than 1.0.7". mbroz, is this second interpretation correct?

comment:20 in reply to: ↑ 19 Changed 5 years ago by mbroz

Replying to toshio:

"Increase library version" as increasing the SONAME

dunno. it is possible that SONAME will change.

(one of the remaining upstream problems are incompatible changes in library breaking ABI between old versions, some distros even do not provide libcryptsetup now because of past problems).

But this problem is unrelated to volume-key, just we need some proper solution to remain compatibilty and avoid old, incompatible versions.

I think that only direct users of libcrypsetup in Fedora are python-cryptsetup and proposed volume-key. Other programs use cryptsetup through commandline. (BTW both maintaners of mentioned packages waits for new functions in libcryptsetup:-)

comment:21 Changed 5 years ago by jstanley

  • Status changed from new to assigned
  • Owner set to jstanley
  • Keywords meeting removed

Approved as temporary solution - leaving this ticket open so we can track removal of the bundled copy

comment:22 follow-up: ↓ 23 Changed 4 years ago by toshio

This looks to have been fixed:

  • Wed Sep 30 2009 Miloslav Trmač <mitr@…> - 0.3-1
  • Update to volume_key-0.3.
  • Drop bundled libcryptsetup.

mitr, care to confirm and we can close this ticket?

comment:23 in reply to: ↑ 22 Changed 4 years ago by mitr

Replying to toshio:

This looks to have been fixed:

  • Wed Sep 30 2009 Miloslav Trmač <mitr@…> - 0.3-1
  • Update to volume_key-0.3.
  • Drop bundled libcryptsetup.

mitr, care to confirm and we can close this ticket?

Yes, the Fedora 12 version does not contain the bundled libcryptsetup any more.

comment:24 Changed 4 years ago by toshio

  • Resolution set to fixed
  • Status changed from assigned to closed

Thanks!

Note: See TracTickets for help on using tickets.