On Thursday, May 06 2021, Didier Roche wrote: > [Summary] > The package is in a very good shape, higher than most of the ones we > review. I have few questions but no big blocker. I feel it would be > still good for the security team (even if they already support it to > some point) to have a second look before we promote it to > main. Hopefully, as this is midly-supported already, that should be a > quick pass.
Thank you very much for the review, Didier :-). > Thanks for the detailed security and CVE analysis, including the > vendored dependencies. Much appreciated :) > > Notes: > Questions: > - I think we should promote it once github.com/gogo/protobuf is fixed and an > upload with the vendored updated dep is done. It seems to be fixed in > 1.18.1+ds1-0ubuntu1, correct? That is correct. The latest version on Impish already has the fix for the gogo/protobuf CVE. Upstream has already fixed it, too. > - Same with github.com/prometheus/prometheus/, let’s wait then for the > latest version of telegraf which isn’t impacted by it (do you have the > version handy? Is it 1.18.1? and so, we can mark this as "DONE"?) The prometheus vulnerability does not affect the version of telegraf that is available on Impish, so I'd consider that "DONE" (or even "INVALID") :-). > - About the github.com/hashicorp/consul CVEs and fixes, do you have > any ETA? I think we should wait for them to be fixed before the actual > promotion (and this can give some time for the security team to assess > the package again), wdyt? Upstream has fixed the CVEs (by updating the hashicorp/consul version they're using) yesterday! I am just waiting for them to cut a release and then I will update the Impish package. > - You are patching the upstream service file in > debian/patches/adjust-service-user.patch but still provides a service > file in debian/telegraf.service. I didn’t see the later installed by > any script, and so, it seems the debian/ one is not needed anymore. Do > you mind having a look and clean that up? (Either removing the patch > which is not needed if we don’t install the upstream one or the unused > .service in debian/) The telegraf.service file under the debian/ directory is actually a symlink to the upstream one; it is needed because dh_installsystemd looks for it when deciding what to install. The patch is needed because we want the telegraf system user to be prefixed by an underscore, following the latest Debian Policy guidelines. So, in summary: both the patch and the symlink are needed :-). > Required TODOs: > - Can you check and update debian/copyright please? Some years are not > present, and copyright attribution are wrong (for instance > vendor/honnef.co/* has some files "Copyright 2014 The Go Authors"). I > think a second look would be good if you can ensure that everything is > up to date. Yes, sure. This will take some time but then again I am waiting until upstream releases a new version anyway :-). > - The package is in the list of lto-disabled list (see > https://launchpad.net/ubuntu/+source/lto-disabled-list). You need to > fix or work-around it directly in the package. If you need an example > for a go package disabling it (due to Go internals): > https://github.com/ubuntu/adsys/blob/main/debian/rules#L11 Ah, good point. I will look into this, thanks. Thanks again for the thorough review! -- Sergio GPG key ID: E92F D0B3 6B14 F1F4 D8E0 EB2F 106D A1C8 C3CB BF14 -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/1926321 Title: [MIR] telegraf To manage notifications about this bug go to: https://bugs.launchpad.net/ubuntu/+source/telegraf/+bug/1926321/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
