Bug#856210: libdebian-installer: please parse SHA256 field and add it to di_* structs

2017-03-01 Thread Bastian Blank 
On Wed, Mar 01, 2017 at 02:25:12PM +0100, Julien Cristau wrote:
> Changing semantics of an existing struct member is classic ABI breakage.
>  This does very much need a SONAME bump.

Technically yes.  But this one is noe used uncontrolled outside.  So it
works without.

Bastian

-- 
Warp 7 -- It's a law we can live with.

Bug#856210: libdebian-installer: please parse SHA256 field and add it to di_* structs

2017-03-01 Thread Steven Chamberlain 
Control: tags -1 - patch

I've no more desire to work on this bug, sorry.

Thanks for the feedback that was given, but it has already been more
effort than I have time or patience to contribute.

For what should be essentially, s/md5/sha256/ in a few places, the
required procedures and personal interactions beyond that, is just too
much.  That is the real "debacle";  it is not surprising to me now, that
this did not happen yet in 10 years, or that no enthusiastic new
contributor had already done this.

Regards,
-- 
Steven Chamberlain
ste...@pyro.eu.org


signature.asc
Description: Digital signature

Processed: Re: Bug#856210: libdebian-installer: please parse SHA256 field and add it to di_* structs

2017-03-01 Thread Debian Bug Tracking System 
Processing control commands:

> tags -1 - patch
Bug #856210 [src:libdebian-installer] libdebian-installer: please parse SHA256 
field and add it to di_* structs
Removed tag(s) patch.

-- 
856210: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=856210
Debian Bug Tracking System
Contact ow...@bugs.debian.org with problems

Bug#856210: libdebian-installer: please parse SHA256 field and add it to di_* structs

2017-03-01 Thread Julien Cristau 
On 02/27/2017 04:40 PM, Steven Chamberlain wrote:
> Bastian Blank wrote:
>> This change breaks the existing ABI and therefor needs an ABI bump, but
>> it is missing from the patch.
> 
> The attached patch tries to bump the soname to 5.  This makes the diff
> much larger, but the code changes are the same.
> 
> I think libdebian-installer-extra nowadays gets a soname bump at the
> same time as libdebian-installer (whereas in the past it was possible to
> set a different soname for each).
> 
> (If we really wanted, we could maybe avoid the ABI bump:  no library
> functions are being added/removed, only the name and meaning of a struct
> member (a pointer, which remains the same length).  The
> dynamically-sized buffer it points to, would change from storing an MD5
> to a SHA256 hash, and would only cause a regression where something is
> still trying to validate MD5).
> 
Changing semantics of an existing struct member is classic ABI breakage.
 This does very much need a SONAME bump.

Cheers,
Julien

Bug#856210: libdebian-installer: please parse SHA256 field and add it to di_* structs

2017-02-28 Thread Steven Chamberlain 
Bastian Blank wrote:
> On Tue, Feb 28, 2017 at 10:00:01PM +, Steven Chamberlain wrote:
> > That differs from the latest version of my patch, and from what I sent
> > earlier today to the release team when asking about a potential unblock:
> > https://lists.debian.org/debian-release/2017/02/msg01033.html
> 
> This happens if you send incomplete patches and do uncoordinated unblock
> requests.

Maybe you just volunteered to do that, then.

You even said before you "don't have time" to write the cdebootstrap
patch, so I offered one, and the anna patch, the libdebian-installer
patch, all this after the initial discovery, triage and write-up.

Regards,
-- 
Steven Chamberlain
ste...@pyro.eu.org


signature.asc
Description: Digital signature

Bug#856210: libdebian-installer: please parse SHA256 field and add it to di_* structs

2017-02-28 Thread Bastian Blank 
On Tue, Feb 28, 2017 at 10:00:01PM +, Steven Chamberlain wrote:
> That differs from the latest version of my patch, and from what I sent
> earlier today to the release team when asking about a potential unblock:
> https://lists.debian.org/debian-release/2017/02/msg01033.html

This happens if you send incomplete patches and do uncoordinated unblock
requests.

Bastian

-- 
Bones: "The man's DEAD, Jim!"

Bug#856210: libdebian-installer: please parse SHA256 field and add it to di_* structs

2017-02-28 Thread Steven Chamberlain 
Bastian Blank wrote:
> Adopted and commited to
> https://anonscm.debian.org/git/d-i/libdebian-installer.git, branch
> sha256

That differs from the latest version of my patch, and from what I sent
earlier today to the release team when asking about a potential unblock:
https://lists.debian.org/debian-release/2017/02/msg01033.html

I think we should wait for them to answer before doing anything else.

Based on KiBi's feedback I thought it better to swap sum[0] and sum[1],
and remove the SHA1 parsing also.

Regards,
-- 
Steven Chamberlain
ste...@pyro.eu.org


signature.asc
Description: Digital signature

Bug#856210: libdebian-installer: please parse SHA256 field and add it to di_* structs

2017-02-28 Thread Bastian Blank 
On Sun, Feb 26, 2017 at 06:30:31PM +, Steven Chamberlain wrote:
> I've attached only the most minimal patch to allow reverse-depends do
> implement SHA256.  They must adapt to the new names of struct members
> *and* remember that the hash length is now different.  (The hash data is
> stored in variable-length fields but the length is not recorded in the
> structs, and the has is denoted by a magic number not an enum;  that
> could be made better, but requiring a much larger diff).

Adopted and commited to
https://anonscm.debian.org/git/d-i/libdebian-installer.git, branch
sha256

Bastian

-- 
Totally illogical, there was no chance.
-- Spock, "The Galileo Seven", stardate 2822.3

Bug#856210: libdebian-installer: please parse SHA256 field and add it to di_* structs

2017-02-28 Thread Cyril Brulebois 
Hi,

Steven Chamberlain  (2017-02-28):
> Changing the name, causes reverse-deps using that field to FTBFS.  I
> think that is just anna and cdebootstrap, which we'd patch anyway.

Sure.

> The md5sum/sha256 field is a pointer to a dynamically-allocated field.
> The struct size, and the offset of other members does not change, so
> nothing else should need rebuilding with the newer package.h
> 
> "If" somehow, we missed something, which tries to dereference
> package->md5sum at run-time with a new version of libdebian-installer,
> it would find a sha256 hash there instead of md5.  That should fail
> "safely" by complaining of a md5sum mismatch (even if it only compares
> the first 32 bytes, as cdebootstrap does currently).
> 
> That's why I think an ABI bump could be safely avoided.  (And I think
> Bastian agrees now?)

I only glanced quickly over the “minimal patch” you sent as a follow-up,
and I think that should do just fine at this point of the release cycle,
yes.

Maybe Bastian will comment before I do (not sure I'll be able to look
into it before a few days).

> > Bumping the ABI seems reasonable to me, even if that's effectively
> > starting a mini-transition from a release point of view.
> 
> [...]
> 
> > > -Package: libdebian-installer4-dev
> > > +Package: libdebian-installer5-dev
> > 
> > Please don't!
> 
> You suggest to "bump the ABI" but not rename the packages?  or...?

But not rename *this* particular binary.

There's no reason to have a versioned -dev package, unless you're
maintaining various src:fooX, src:fooY packages at the same time, and so
that one can choose between libfooX-dev and libfooY-dev (hello openssl).
That's not what's happening here.

> Maybe the argument above is convincing enough to just not bump the ABI?
> 
> > > --- a/include/debian-installer/release.h
> > > +++ b/include/debian-installer/release.h
> > > @@ -40,7 +40,7 @@ struct di_release
> > >char *origin; /**< Origin field */
> > >char *suite;  /**< Suite field */
> > >char *codename;   /**< Codename field */
> > > -  di_hash_table *md5sum;/**< checksum fields, 
> > > includes di_release_file */
> > > +  di_hash_table *sha256;/**< checksum fields, 
> > > includes di_release_file */
> > >di_mem_chunk *release_file_mem_chunk; /**< @internal */
> > >  };
> > 
> > So md5sum goes away from the di_release struct…
> 
> Yes, the same as with di_package;  that preserves ABI compatibility,
> and getting rid of md5sum is also our intent.

FWIW I'm not sure I'm convinced changing semantics for a given field can
be advertised as keeping “ABI compatibility” (even if one can decide to
ignore this issue).

> > > @@ -55,7 +55,7 @@ struct di_release_file
> > >  di_rstring key; /**< @internal */
> > >};
> > >unsigned int size;/**< size */
> > > -  char *sum[2]; /**< checksums, 
> > > currently md5 and sha1 */
> > > +  char *sum[2]; /**< checksums, 
> > > currently md5 and sha256 */
> > 
> > … but is kept in the di_release_file one?
> 
> Right, this struct currently contains:
> 
> char *sum[0] -> dynamically allocated md5sum field
> char *sum[1] -> dynamically allocated sha1 field
> 
> so that is what reverse-depends expect to be in those fields,
> currently.  To keep ABI comptibility, I should keep two items there.

Well, your initial patch was bumping the ABI, so it looked to me like it
could have been cleaned up at the same time, and that's why I asked. But
nevermind, going a different route now. Someone can rethink this with a
dynamic checksum mapping in a later release (see people/waldi branch).


KiBi.


signature.asc
Description: Digital signature

Bug#856210: libdebian-installer: please parse SHA256 field and add it to di_* structs

2017-02-28 Thread Steven Chamberlain 
Steven Chamberlain wrote:
> replace sum[0] with sha256 and leave sum[1] empty; 
> [...] (we would drop the MD5- and SHA1-parsing code
> and make absolutely sure nobody is still using those).

The new patch attached would do that, and it remains otherwise
ABI-compatible.

It aims to be the most minimal diff, so it does not extend the testsuite
for example, which still passes even though the Packages file testcase
has no SHA256 fields.

In src/release.c:  file->sum[1] is initialised to NULL by a calloc().
In the future, someone might want to put SHA512 hashes there.  It does
not hurt to keep the existing di_free(file->sum[1]) in place.

Within the installer, this should only break anna, until the patch from
#856211 is applied.

Outside of the installer, cdebootstrap would break, until #856212 is
patched.

If we missed any other reverse-depends, they should FTBFS if they
dereference the md5sum fields.  Already-built binaries should report a
"md5sum mismatch", if they use the patched libdebian-installer at
run-time and still try to do verification with MD5.

Regards,
-- 
Steven Chamberlain
ste...@pyro.eu.org
diff --git a/debian/changelog b/debian/changelog
index 3dd29e1..748a78b 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,7 +1,17 @@
 libdebian-installer (0.109) UNRELEASED; urgency=medium
 
+  [ Samuel Thibault ]
   * Fix build with gcc-7. Closes: #853489
 
+  [ Steven Chamberlain ]
+  * In structs di_release and di_package, replace md5sum member with a
+sha256 member.  This is ABI-compatible, but reverse-dependencies
+will fail if they still try to verify with MD5 (Closes: #856212).
+  * Parse SHA256 fields instead of MD5Sum fields in Packages files.
+  * Parse SHA256 fields instead of MD5Sum fields in Release files.
+  * No longer try to parse SHA1 field, which is no longer present in
+Release files as of stretch.
+
  -- Samuel Thibault   Tue, 31 Jan 2017 11:09:16 +0100
 
 libdebian-installer (0.108) unstable; urgency=medium
diff --git a/include/debian-installer/package.h b/include/debian-installer/package.h
index 72d7444..e1f699d 100644
--- a/include/debian-installer/package.h
+++ b/include/debian-installer/package.h
@@ -112,7 +112,7 @@ struct di_package
   di_slist depends; /**< Any different dependency types */
   char *filename;   /**< Filename field */
   size_t size;  /**< Size field */
-  char *md5sum; /**< MD5Sum field */
+  char *sha256; /**< SHA256 field */
   char *short_description;  /**< Description field, first part*/
   char *description;/**< Description field, second part */
   unsigned int resolver;/**< @internal */
diff --git a/include/debian-installer/package_internal.h b/include/debian-installer/package_internal.h
index f6357d1..d410ce2 100644
--- a/include/debian-installer/package_internal.h
+++ b/include/debian-installer/package_internal.h
@@ -52,7 +52,7 @@ const di_parser_fieldinfo
   internal_di_package_parser_field_enhances,
   internal_di_package_parser_field_filename,
   internal_di_package_parser_field_size,
-  internal_di_package_parser_field_md5sum,
+  internal_di_package_parser_field_sha256,
   internal_di_package_parser_field_description;
 
 /**
diff --git a/include/debian-installer/release.h b/include/debian-installer/release.h
index 223a4f8..29b0cfb 100644
--- a/include/debian-installer/release.h
+++ b/include/debian-installer/release.h
@@ -40,7 +40,7 @@ struct di_release
   char *origin; /**< Origin field */
   char *suite;  /**< Suite field */
   char *codename;   /**< Codename field */
-  di_hash_table *md5sum;/**< checksum fields, includes di_release_file */
+  di_hash_table *sha256;/**< checksum fields, includes di_release_file */
   di_mem_chunk *release_file_mem_chunk; /**< @internal */
 };
 
@@ -55,7 +55,7 @@ struct di_release_file
 di_rstring key; /**< @internal */
   };
   unsigned int size;/**< size */
-  char *sum[2]; /**< checksums, currently md5 and sha1 */
+  char *sum[2]; /**< sum[0] is sha256, sum[1] is empty */
 };
 
 di_release *di_release_alloc (void);
diff --git a/src/package.c b/src/package.c
index 653b5dd..82c7653 100644
--- a/src/package.c
+++ b/src/package.c
@@ -38,7 +38,7 @@ void di_package_destroy (di_package *package)
   di_free (package->architecture);
   di_free (package->version);
   di_free (package->filename);
-  di_free (package->md5sum);
+  di_free (package->sha256);
   di_free (package->short_description);
   di_free (package->description);

Bug#856210: libdebian-installer: please parse SHA256 field and add it to di_* structs

2017-02-28 Thread Steven Chamberlain 
Thanks for your comments!

Cyril Brulebois wrote:
> Steven Chamberlain  (2017-02-27):
> > (If we really wanted, we could maybe avoid the ABI bump: [...]
> 
> Given the number of reverse dependencies, I doubt this is worth abusing
> md5 storage for sha256 things.

Maybe I should clarify that;  the current libdebian-installer/0.108 has:

di_package struct {
...
char *md5sum; // -> dynamically allocated md5sum field
...
}

and we'd be changing it to:

di_package struct {
...
char *sha256; // -> dynamically allocated sha256 field
...
}

("Sum" was dropped from name of that field in the Release file, so I do
the same here)

Changing the name, causes reverse-deps using that field to FTBFS.  I
think that is just anna and cdebootstrap, which we'd patch anyway.

The md5sum/sha256 field is a pointer to a dynamically-allocated field.
The struct size, and the offset of other members does not change, so
nothing else should need rebuilding with the newer package.h

"If" somehow, we missed something, which tries to dereference
package->md5sum at run-time with a new version of libdebian-installer,
it would find a sha256 hash there instead of md5.  That should fail
"safely" by complaining of a md5sum mismatch (even if it only compares
the first 32 bytes, as cdebootstrap does currently).

That's why I think an ABI bump could be safely avoided.  (And I think
Bastian agrees now?)

> Bumping the ABI seems reasonable to me,
> even if that's effectively starting a mini-transition from a release
> point of view.

[...]

> > -Package: libdebian-installer4-dev
> > +Package: libdebian-installer5-dev
> 
> Please don't!

You suggest to "bump the ABI" but not rename the packages?  or...?
Maybe the argument above is convincing enough to just not bump the ABI?

> > --- a/include/debian-installer/release.h
> > +++ b/include/debian-installer/release.h
> > @@ -40,7 +40,7 @@ struct di_release
> >char *origin; /**< Origin field */
> >char *suite;  /**< Suite field */
> >char *codename;   /**< Codename field */
> > -  di_hash_table *md5sum;/**< checksum fields, 
> > includes di_release_file */
> > +  di_hash_table *sha256;/**< checksum fields, 
> > includes di_release_file */
> >di_mem_chunk *release_file_mem_chunk; /**< @internal */
> >  };
> 
> So md5sum goes away from the di_release struct…

Yes, the same as with di_package;  that preserves ABI compatibility,
and getting rid of md5sum is also our intent.

> 
> >  
> > @@ -55,7 +55,7 @@ struct di_release_file
> >  di_rstring key; /**< @internal */
> >};
> >unsigned int size;/**< size */
> > -  char *sum[2]; /**< checksums, currently 
> > md5 and sha1 */
> > +  char *sum[2]; /**< checksums, currently 
> > md5 and sha256 */
> 
> … but is kept in the di_release_file one?

Right, this struct currently contains:

char *sum[0] -> dynamically allocated md5sum field
char *sum[1] -> dynamically allocated sha1 field

so that is what reverse-depends expect to be in those fields,
currently.  To keep ABI comptibility, I should keep two items there.

The sha1 field is always empty, since that was removed from the Release
file.  We could either:

  1. replace sum[0] with sha256 and leave sum[1] empty;  or
  2. leave sum[0] containing md5 but replace sum[1] with sha256

My patch did 2. because it results in a smaller diff.  But I like the
idea of doing 1. instead (we would drop the MD5- and SHA1-parsing code
and make absolutely sure nobody is still using those).

If I did 1. and we didn't bump the ABI, it should be easy to test:
  * we'd patch+update only libdebian-installer, then test:
anna should abort the install, due to mismatching md5sums;
  * then we'd patch anna, and it should all work again;  one could
also delete the /usr/bin/md5sum symlink while testing.

Regards,
-- 
Steven Chamberlain
ste...@pyro.eu.org


signature.asc
Description: Digital signature

Bug#856210: libdebian-installer: please parse SHA256 field and add it to di_* structs

2017-02-27 Thread Bastian Blank 
On Tue, Feb 28, 2017 at 04:11:50AM +0100, Cyril Brulebois wrote:
> > (If we really wanted, we could maybe avoid the ABI bump:  no library
> > functions are being added/removed, only the name and meaning of a struct
> > member (a pointer, which remains the same length).  The
> > dynamically-sized buffer it points to, would change from storing an MD5
> > to a SHA256 hash, and would only cause a regression where something is
> > still trying to validate MD5).
> 
> Given the number of reverse dependencies, I doubt this is worth abusing
> md5 storage for sha256 things. Bumping the ABI seems reasonable to me,
> even if that's effectively starting a mini-transition from a release
> point of view.

On second thought, let's just do it without ABI name change.  For d-i
breaks don't work well, but if we update them en block this will not
show any breakage.  For the rest (exactl one user) breaks works fine.

Bastian

-- 
It would be illogical to assume that all conditions remain stable.
-- Spock, "The Enterprise Incident", stardate 5027.3

Bug#856210: libdebian-installer: please parse SHA256 field and add it to di_* structs

2017-02-27 Thread Cyril Brulebois 
Steven Chamberlain  (2017-02-27):
> The attached patch tries to bump the soname to 5.  This makes the diff
> much larger, but the code changes are the same.

Thanks. Some comments below.

> I think libdebian-installer-extra nowadays gets a soname bump at the
> same time as libdebian-installer (whereas in the past it was possible
> to set a different soname for each).

Probably fine to bump both at once.

> (If we really wanted, we could maybe avoid the ABI bump:  no library
> functions are being added/removed, only the name and meaning of a struct
> member (a pointer, which remains the same length).  The
> dynamically-sized buffer it points to, would change from storing an MD5
> to a SHA256 hash, and would only cause a regression where something is
> still trying to validate MD5).

Given the number of reverse dependencies, I doubt this is worth abusing
md5 storage for sha256 things. Bumping the ABI seems reasonable to me,
even if that's effectively starting a mini-transition from a release
point of view.

FWIW, out of all d-i packages, only anna seems to be accessing the
->md5sum member.

> +  [ Steven Chamberlain ]
> +  * Parse SHA256 fields instead of MD5Sum fields in Packages files.
> +  * Parse SHA256 fields instead of (no longer existing) SHA1 fields in
> +Release files.
> +  * In structs di_release and di_package, add new sha256 member and
> +remove the md5sum member (a backward-incompatible change, this will
> +force reverse-dependencies to stop using MD5 for verification)
> +(Closes: #856212).
> +  * Bump soname as advised by Bastian Blank.
> +
>   -- Samuel Thibault   Tue, 31 Jan 2017 11:09:16 +0100
>  
>  libdebian-installer (0.108) unstable; urgency=medium
> diff --git a/debian/control b/debian/control
> index 0949fd9..f53f55c 100644
> --- a/debian/control
> +++ b/debian/control
> @@ -8,7 +8,7 @@ Standards-Version: 3.9.6
>  Vcs-Browser: https://anonscm.debian.org/cgit/d-i/libdebian-installer.git
>  Vcs-Git: https://anonscm.debian.org/git/d-i/libdebian-installer.git
>  
> -Package: libdebian-installer4
> +Package: libdebian-installer5
>  Architecture: any
>  Multi-Arch: same
>  Pre-Depends: ${misc:Pre-Depends}
> @@ -19,10 +19,10 @@ Description: Library of common debian-installer functions
>   working on debian-installer or building your own install system based
>   on debian-installer, then you probably don't need this library.
>  
> -Package: libdebian-installer4-dev
> +Package: libdebian-installer5-dev

Please don't! We're not going to support multiple libfooN-dev at the
same time. If it were to be renamed, this should become for an
unversioned libdebian-installer-dev. And now is definitely not the
time for such a thing.

And of course, this would make packages unbuildable anyway, given
libdebian-installer4-dev appears in Build-Depends for a bunch of
packages.

>  Section: libdevel
>  Architecture: any
> -Depends: ${misc:Depends}, libdebian-installer4 (= ${binary:Version}), 
> libdebian-installer-extra4 (= ${binary:Version})
> +Depends: ${misc:Depends}, libdebian-installer5 (= ${binary:Version}), 
> libdebian-installer-extra5 (= ${binary:Version})
>  Conflicts: libdebian-installer-dev
>  Provides: libdebian-installer-dev
>  Description: Library of common debian-installer functions
> @@ -33,7 +33,7 @@ Description: Library of common debian-installer functions
>   .
>   This package contains files needed to do libdebian-installer development.
>  
> -Package: libdebian-installer4-udeb
> +Package: libdebian-installer5-udeb
>  Package-Type: udeb
>  Section: debian-installer
>  Architecture: any
> @@ -44,22 +44,22 @@ Description: Library of common debian-installer functions
>   working on debian-installer or building your own install system based
>   on debian-installer, then you probably don't need this library.
>  
> -Package: libdebian-installer-extra4
> +Package: libdebian-installer-extra5
>  Architecture: any
>  Multi-Arch: same
> -Depends: ${shlibs:Depends}, ${misc:Depends}, libdebian-installer4 (= 
> ${binary:Version})
> +Depends: ${shlibs:Depends}, ${misc:Depends}, libdebian-installer5 (= 
> ${binary:Version})
>  Description: Library of some extra debian-installer functions
>   This library is used by debian-installer to perform common functions
>   such as logging messages and executing commands. If you aren't
>   working on debian-installer or building your own install system based
>   on debian-installer, then you probably don't need this library.
>  
> -Package: libdebian-installer-extra4-udeb
> +Package: libdebian-installer-extra5-udeb
>  Package-Type: udeb
>  Section: debian-installer
>  Architecture: any
> -Depends: ${shlibs:Depends}, ${misc:Depends}, libdebian-installer4-udeb (= 
> ${binary:Version})
> -Provides: libdebian-installer-extra4
> +Depends: ${shlibs:Depends}, ${misc:Depends}, libdebian-installer5-udeb (= 
> ${binary:Version})
> +Provides: libdebian-installer-extra5
>  Description: Library of some extra

Bug#856210: libdebian-installer: please parse SHA256 field and add it to di_* structs

2017-02-27 Thread Steven Chamberlain 
Bastian Blank wrote:
> This change breaks the existing ABI and therefor needs an ABI bump, but
> it is missing from the patch.

The attached patch tries to bump the soname to 5.  This makes the diff
much larger, but the code changes are the same.

I think libdebian-installer-extra nowadays gets a soname bump at the
same time as libdebian-installer (whereas in the past it was possible to
set a different soname for each).

(If we really wanted, we could maybe avoid the ABI bump:  no library
functions are being added/removed, only the name and meaning of a struct
member (a pointer, which remains the same length).  The
dynamically-sized buffer it points to, would change from storing an MD5
to a SHA256 hash, and would only cause a regression where something is
still trying to validate MD5).

Regards,
-- 
Steven Chamberlain
ste...@pyro.eu.org
diff --git a/configure.ac b/configure.ac
index d559d64..2f276b8 100644
--- a/configure.ac
+++ b/configure.ac
@@ -16,9 +16,9 @@ AS_IF([test "x$enable_check" != xno],[
 ])
 AM_CONDITIONAL([ENABLE_CHECK],[test "x$enable_check" != xno])
 
-LIBRARY_VERSION_MAJOR=4
+LIBRARY_VERSION_MAJOR=5
 LIBRARY_VERSION_MINOR=0
-LIBRARY_VERSION_REVISION=7
+LIBRARY_VERSION_REVISION=0
 LIBRARY_VERSION="$LIBRARY_VERSION_MAJOR.$LIBRARY_VERSION_MINOR.$LIBRARY_VERSION_REVISION"
 LIBRARY_VERSION_LIBTOOL="$LIBRARY_VERSION_MAJOR:$LIBRARY_VERSION_MINOR:$LIBRARY_VERSION_REVISION"
 
diff --git a/debian/changelog b/debian/changelog
index 3dd29e1..09c194e 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,7 +1,18 @@
 libdebian-installer (0.109) UNRELEASED; urgency=medium
 
+  [ Samuel Thibault ]
   * Fix build with gcc-7. Closes: #853489
 
+  [ Steven Chamberlain ]
+  * Parse SHA256 fields instead of MD5Sum fields in Packages files.
+  * Parse SHA256 fields instead of (no longer existing) SHA1 fields in
+Release files.
+  * In structs di_release and di_package, add new sha256 member and
+remove the md5sum member (a backward-incompatible change, this will
+force reverse-dependencies to stop using MD5 for verification)
+(Closes: #856212).
+  * Bump soname as advised by Bastian Blank.
+
  -- Samuel Thibault   Tue, 31 Jan 2017 11:09:16 +0100
 
 libdebian-installer (0.108) unstable; urgency=medium
diff --git a/debian/control b/debian/control
index 0949fd9..f53f55c 100644
--- a/debian/control
+++ b/debian/control
@@ -8,7 +8,7 @@ Standards-Version: 3.9.6
 Vcs-Browser: https://anonscm.debian.org/cgit/d-i/libdebian-installer.git
 Vcs-Git: https://anonscm.debian.org/git/d-i/libdebian-installer.git
 
-Package: libdebian-installer4
+Package: libdebian-installer5
 Architecture: any
 Multi-Arch: same
 Pre-Depends: ${misc:Pre-Depends}
@@ -19,10 +19,10 @@ Description: Library of common debian-installer functions
  working on debian-installer or building your own install system based
  on debian-installer, then you probably don't need this library.
 
-Package: libdebian-installer4-dev
+Package: libdebian-installer5-dev
 Section: libdevel
 Architecture: any
-Depends: ${misc:Depends}, libdebian-installer4 (= ${binary:Version}), libdebian-installer-extra4 (= ${binary:Version})
+Depends: ${misc:Depends}, libdebian-installer5 (= ${binary:Version}), libdebian-installer-extra5 (= ${binary:Version})
 Conflicts: libdebian-installer-dev
 Provides: libdebian-installer-dev
 Description: Library of common debian-installer functions
@@ -33,7 +33,7 @@ Description: Library of common debian-installer functions
  .
  This package contains files needed to do libdebian-installer development.
 
-Package: libdebian-installer4-udeb
+Package: libdebian-installer5-udeb
 Package-Type: udeb
 Section: debian-installer
 Architecture: any
@@ -44,22 +44,22 @@ Description: Library of common debian-installer functions
  working on debian-installer or building your own install system based
  on debian-installer, then you probably don't need this library.
 
-Package: libdebian-installer-extra4
+Package: libdebian-installer-extra5
 Architecture: any
 Multi-Arch: same
-Depends: ${shlibs:Depends}, ${misc:Depends}, libdebian-installer4 (= ${binary:Version})
+Depends: ${shlibs:Depends}, ${misc:Depends}, libdebian-installer5 (= ${binary:Version})
 Description: Library of some extra debian-installer functions
  This library is used by debian-installer to perform common functions
  such as logging messages and executing commands. If you aren't
  working on debian-installer or building your own install system based
  on debian-installer, then you probably don't need this library.
 
-Package: libdebian-installer-extra4-udeb
+Package: libdebian-installer-extra5-udeb
 Package-Type: udeb
 Section: debian-installer
 Architecture: any
-Depends: ${shlibs:Depends}, ${misc:Depends}, libdebian-installer4-udeb (= ${binary:Version})
-Provides: libdebian-installer-extra4
+Depends: ${shlibs:Depends}, ${misc:Depends}, libdebian-installer5-udeb (= ${binary:Version})
+Provides: libdebian-installer-extra5
 Description: Library of some

Re: Bug#856210: libdebian-installer: please parse SHA256 field and add it to di_* structs

2017-02-27 Thread Cyril Brulebois 
Hi,

Steven Chamberlain  (2017-02-26):
> Colin Watson wrote:
> > Just FYI, since it's not clear from
> > https://wiki.debian.org/InstallerDebacle that you know this, the
> > installer in fact uses debootstrap rather than cdebootstrap to install
> > the base system.
> 
> I didn't realise that, thanks.  There was still a cdebootstrap-udeb in
> wheezy, so that installer is affected?  But not releases since.

Well, I've only been doing d-i releases for a few cycles, but base-installer's
history shows no cdebootstrap-udeb in its Depends, ever; granted, history
starts at this point:
| commit 5203c4b49f36c4372de948f6b3edc1b9c4041a7a
| Author: Tollef Fog Heen 
| Date:   Sat Apr 27 19:16:08 2002 +
| 
| Initial checkin
| 
| r637

> base-installer seems it would (still now) use it in preference to
> regular debootstrap, *if* it was available in the installer:
> http://sources.debian.net/src/base-installer/1.168/debian/bootstrap-base.postinst/?hl=145#L145

The only reference I see in debian-installer's history is its being added in
post-sarge goals, before being removed again, so it looks to me it would only
be put there by people who are supposed to know what they're doing?

Adding to this my initial comments about anna and net-retriever, I think it
might have made sense to be a little less clickbaity with the whole “debacle”
title. While there's clearly room for improvements in various components, I'm
not sure the installation process is as broken as you made it to be.



KiBi.


signature.asc
Description: Digital signature

Bug#856210: libdebian-installer: please parse SHA256 field and add it to di_* structs

2017-02-26 Thread Steven Chamberlain 
Hi,

Bastian Blank wrote:
> This change breaks the existing ABI and therefor needs an ABI bump, but
> it is missing from the patch.

I agree, that should be done.  Thanks.

Regards,
-- 
Steven Chamberlain
ste...@pyro.eu.org


signature.asc
Description: Digital signature

Re: Bug#856210: libdebian-installer: please parse SHA256 field and add it to di_* structs

2017-02-26 Thread Steven Chamberlain 
Hi,

Colin Watson wrote:
> Just FYI, since it's not clear from
> https://wiki.debian.org/InstallerDebacle that you know this, the
> installer in fact uses debootstrap rather than cdebootstrap to install
> the base system.

I didn't realise that, thanks.  There was still a cdebootstrap-udeb in
wheezy, so that installer is affected?  But not releases since.

base-installer seems it would (still now) use it in preference to
regular debootstrap, *if* it was available in the installer:
http://sources.debian.net/src/base-installer/1.168/debian/bootstrap-base.postinst/?hl=145#L145

Do you know any places where cdebootstrap is still used?  (It is still
having new features added in the past months, so it may not be an option
to simply remove it from the stable release).

I found a random example in "gitlab-ci-multi-runner"
http://sources.debian.net/src/gitlab-ci-multi-runner/1.10.3%2Bdfsg-1/debian/mk-prebuilt-images.sh.in/?hl=62#L62

> AFAIK debootstrap has supported SHA256 since version
> 1.0.28 in 2011.

I looked at debootstrap in sid and it seems unaffected by these issues,
yes.

> >   + allow verifiers to check both MD5 *and* SHA256, for even stronger
> > authentication in case one or both algorithms are broken
> 
> Checking both adds only negligible security (look up "multicollisions")
> and is a waste of time.

I wouldn't dismiss it for that reason, but I think it adds such
complexity that we would likely make some more serious error, if we
tried.

> The usual reason to keep support for older hash algorithms is just to
> make transitions to newer ones less painful.

Maybe it makes sense to do that on the archive side (add new hash
algorithms before removing old ones);  but to do that here, in the
consuming utility, has turned out quite harmful, in retrospect.

From this, I would conclude that cdebootstrap should have dropped
all support for MD5 when SHA1 support was added, i.e. require a SHA1
field, and fail loudly if it's not there;  and prune out all of the MD5
code (which might have avoided #856213).

I think archive utils have had plenty of time (10 years!) to add SHA256
fields, so it is reasonable now to require a SHA256 field be present,
and validate only that?

Regards,
-- 
Steven Chamberlain
ste...@pyro.eu.org


signature.asc
Description: Digital signature

Re: Bug#856210: libdebian-installer: please parse SHA256 field and add it to di_* structs

2017-02-26 Thread Colin Watson 
On Sun, Feb 26, 2017 at 06:30:31PM +, Steven Chamberlain wrote:
> The regression in Bug#856215 in cdebootstrap:
> "since SHA1 removal from Release file, only MD5sums are used"
> could only be fixed by adding support for the SHA256 fields.

Just FYI, since it's not clear from
https://wiki.debian.org/InstallerDebacle that you know this, the
installer in fact uses debootstrap rather than cdebootstrap to install
the base system.  AFAIK debootstrap has supported SHA256 since version
1.0.28 in 2011.

> An open question is whether to preserve any support for MD5.
> Keeping it would:
> 
>   + reduce potential for breakage (in case MD5 is "good enough" for some
> use-case or SHA256 is still impractical)
>   + allow verifiers to check both MD5 *and* SHA256, for even stronger
> authentication in case one or both algorithms are broken

Checking both adds only negligible security (look up "multicollisions")
and is a waste of time.

The usual reason to keep support for older hash algorithms is just to
make transitions to newer ones less painful.

-- 
Colin Watson   [cjwat...@debian.org]

Bug#856210: libdebian-installer: please parse SHA256 field and add it to di_* structs

2017-02-26 Thread Bastian Blank 
Hi Steven

On Sun, Feb 26, 2017 at 06:30:31PM +, Steven Chamberlain wrote:
> I've attached only the most minimal patch to allow reverse-depends do
> implement SHA256.  They must adapt to the new names of struct members
> *and* remember that the hash length is now different.  (The hash data is
> stored in variable-length fields but the length is not recorded in the
> structs, and the has is denoted by a magic number not an enum;  that
> could be made better, but requiring a much larger diff).

This change breaks the existing ABI and therefor needs an ABI bump, but
it is missing from the patch.

Regards,
Bastian

-- 
It is necessary to have purpose.
-- Alice #1, "I, Mudd", stardate 4513.3

Bug#856210: libdebian-installer: please parse SHA256 field and add it to di_* structs

2017-02-26 Thread Steven Chamberlain 
With that patch, reverse-deps anna and cdebootstrap shall FTBFS with:

| gcc -Wdate-time -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE -g -O2 
-fdebug-prefix-map=/home/steven/git/anna=. 
-specs=/usr/share/dpkg/pie-compile.specs -fstack-protector-strong -Wformat 
-Werror=format-security -Wall -W -ggdb -Wdate-time -D_FORTIFY_SOURCE=2 
-D_GNU_SOURCE  -c -o anna.o anna.c
| anna.c: In function ‘install_modules’:
| anna.c:321:25: error: ‘di_package {aka struct di_package}’ has no member 
named ‘md5sum’
|  if (! md5sum(package->md5sum, dest_file)) {
|  ^~

| gcc -DHAVE_CONFIG_H -I. -I../../src -I..  -I../../include -Wdate-time 
-D_FORTIFY_SOURCE=2  -g -O2 
-fdebug-prefix-map=/home/steven/git/cdebootstrap-0.7.6=. 
-specs=/usr/share/dpkg/pie-compile.specs -fstack-protector-strong -Wformat 
-Werror=format-security -std=gnu99 -c -o gpg.o ../../src/gpg.c
| ../../src/check.c: In function ‘check_deb’:
| ../../src/check.c:61:40: error: ‘di_package {aka struct di_package}’ has no 
member named ‘md5sum’
|return check_sum (target, "md5sum", p->md5sum, message);
| ^~
| ../../src/check.c: In function ‘check_packages’:
| ../../src/check.c:75:35: error: ‘di_release {aka struct di_release}’ has no 
member named ‘md5sum’
|item = di_hash_table_lookup (rel->md5sum, );
|^~

so it should be quite clear that they must implement a new hashing
algorithm;  and this makes absolutely sure they are not still using MD5
unintentionally (which was the case in #856215).

If my libdebian-installer patch is okay, I will submit the patches for
anna and cdebootstrap (bugs are already filed against them).  Hopefully
no other reverse-dependencies would be affected (because they do not use
the md5sums field, and the struct size is not changing);  though if they
do use, I'd prefer they FTBFS so that we find out.

Regards,
-- 
Steven Chamberlain
ste...@pyro.eu.org


signature.asc
Description: Digital signature

Bug#856210: libdebian-installer: please parse SHA256 field and add it to di_* structs

2017-02-26 Thread Steven Chamberlain 
Control: tags -1 + patch

Hi,

The regression in Bug#856215 in cdebootstrap:
"since SHA1 removal from Release file, only MD5sums are used"
could only be fixed by adding support for the SHA256 fields.

An open question is whether to preserve any support for MD5.
Keeping it would:

  + reduce potential for breakage (in case MD5 is "good enough" for some
use-case or SHA256 is still impractical)
  + allow verifiers to check both MD5 *and* SHA256, for even stronger
authentication in case one or both algorithms are broken
  - add complexity

Otherwise, dropping MD5 entirely would:

  * break reverse-dependencies (hopefully just anna, cdebootstrap) thus
*forcing* us to stop using MD5 there, and implement SHA256

I've attached only the most minimal patch to allow reverse-depends do
implement SHA256.  They must adapt to the new names of struct members
*and* remember that the hash length is now different.  (The hash data is
stored in variable-length fields but the length is not recorded in the
structs, and the has is denoted by a magic number not an enum;  that
could be made better, but requiring a much larger diff).

A follow-up commit should extend the testsuite to check parsing of the
SHA256 fields;  that also would result in a larger diff however.

Regards,
-- 
Steven Chamberlain
ste...@pyro.eu.org
diff --git a/debian/changelog b/debian/changelog
index 3dd29e1..1b7fcd8 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,7 +1,16 @@
 libdebian-installer (0.109) UNRELEASED; urgency=medium
 
+  [ Samuel Thibault ]
   * Fix build with gcc-7. Closes: #853489
 
+  [ Steven Chamberlain ]
+  * Parse SHA256 fields instead of MD5Sum fields in Packages files.
+  * Parse SHA256 fields instead of (no longer existing) SHA1 fields in
+Release files.
+  * In structs di_release and di_package, add new sha256 member and
+remove the md5sum member (a backward-incompatible change, this will
+force reverse-dependencies to stop using MD5 for verification).
+
  -- Samuel Thibault   Tue, 31 Jan 2017 11:09:16 +0100
 
 libdebian-installer (0.108) unstable; urgency=medium
diff --git a/include/debian-installer/package.h b/include/debian-installer/package.h
index 72d7444..e1f699d 100644
--- a/include/debian-installer/package.h
+++ b/include/debian-installer/package.h
@@ -112,7 +112,7 @@ struct di_package
   di_slist depends; /**< Any different dependency types */
   char *filename;   /**< Filename field */
   size_t size;  /**< Size field */
-  char *md5sum; /**< MD5Sum field */
+  char *sha256; /**< SHA256 field */
   char *short_description;  /**< Description field, first part*/
   char *description;/**< Description field, second part */
   unsigned int resolver;/**< @internal */
diff --git a/include/debian-installer/package_internal.h b/include/debian-installer/package_internal.h
index f6357d1..d410ce2 100644
--- a/include/debian-installer/package_internal.h
+++ b/include/debian-installer/package_internal.h
@@ -52,7 +52,7 @@ const di_parser_fieldinfo
   internal_di_package_parser_field_enhances,
   internal_di_package_parser_field_filename,
   internal_di_package_parser_field_size,
-  internal_di_package_parser_field_md5sum,
+  internal_di_package_parser_field_sha256,
   internal_di_package_parser_field_description;
 
 /**
diff --git a/include/debian-installer/release.h b/include/debian-installer/release.h
index 223a4f8..8e3c572 100644
--- a/include/debian-installer/release.h
+++ b/include/debian-installer/release.h
@@ -40,7 +40,7 @@ struct di_release
   char *origin; /**< Origin field */
   char *suite;  /**< Suite field */
   char *codename;   /**< Codename field */
-  di_hash_table *md5sum;/**< checksum fields, includes di_release_file */
+  di_hash_table *sha256;/**< checksum fields, includes di_release_file */
   di_mem_chunk *release_file_mem_chunk; /**< @internal */
 };
 
@@ -55,7 +55,7 @@ struct di_release_file
 di_rstring key; /**< @internal */
   };
   unsigned int size;/**< size */
-  char *sum[2]; /**< checksums, currently md5 and sha1 */
+  char *sum[2]; /**< checksums, currently md5 and sha256 */
 };
 
 di_release *di_release_alloc (void);
diff --git a/src/package.c b/src/package.c
index 653b5dd..82c7653 100644
--- a/src/package.c
+++ b/src/package.c
@@ -38,7 +38,7 @@ void di_package_destroy (di_package *package)
   di_free (package->architecture);
   di_free (package->version);
   di_free (package->filename);
-  di_free

Processed: Re: Bug#856210: libdebian-installer: please parse SHA256 field and add it to di_* structs

2017-02-26 Thread Debian Bug Tracking System 
Processing control commands:

> tags -1 + patch
Bug #856210 [src:libdebian-installer] libdebian-installer: please parse SHA256 
field and add it to di_* structs
Added tag(s) patch.

-- 
856210: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=856210
Debian Bug Tracking System
Contact ow...@bugs.debian.org with problems

Bug#856210: libdebian-installer: please parse SHA256 field and add it to di_* structs

2017-02-26 Thread Steven Chamberlain 
Source: libdebian-installer
Version: 0.108
Severity: serious
Tags: security
X-Debbugs-Cc: secur...@debian.org
User: debian-rele...@lists.debian.org
Usertags: bsp-2017-02-de-Berlin

Hi,

The 'etch' release (2007) added to the Release file, a field for SHA256
sums to authenticate Packages files.  But to date, libdebian-installer
does not parse it, so anna (which fetches .udeb installer component) and
cdebootstrap (which fetches .deb base system packages) can not yet
verify the SHA256 sums.

http://sources.debian.net/src/libdebian-installer/0.108/include/debian-installer/release.h/#L43
http://sources.debian.net/src/libdebian-installer/0.108/include/debian-installer/release.h/#L58
http://sources.debian.net/src/libdebian-installer/0.108/include/debian-installer/package.h/#L115

Further context and an overview of related bugs will be published at:
https://wiki.debian.org/InstallerDebacle

This bug is not itself RC, but it will be a blocking issue for RC bugs
I'm about to file.

I intend to submit a patch for this shortly.

Thanks,
Regards,
-- 
Steven Chamberlain
ste...@pyro.eu.org


signature.asc
Description: Digital signature