open-eid / DigiDoc4-Client

DigiDoc4 is an application for digitally signing and encrypting documents; the software includes functionality to manage Estonian ID-card - change pin codes etc.
https://www.id.ee/en/article/install-id-software/
Other
120 stars 42 forks source link

bug in DeveloperTips wiki page #1274

Closed Germano0 closed 4 months ago

Germano0 commented 5 months ago

Hello, Fedora package co-maintainer here. There is a bug in the DeveloperTips wiki page: the section

Replace line  
https://github.com/open-eid/qt-common/blob/master/CMakeLists.txt#L57   
`qt_add_resources(CONFIG_SOURCES ${CMAKE_CURRENT_BINARY_DIR}/config.qrc)`   
with   
`qt_add_resources(CONFIG_SOURCES config.qrc)` 

contains a link that does not point to the

`qt_add_resources(CONFIG_SOURCES ${CMAKE_CURRENT_BINARY_DIR}/config.qrc)`   

line. Instead it points to

target_compile_definitions(qdigidoccommon PUBLIC CONFIG_URL="${CONFIG_URL}")

line. Also, the current approach to CMake usage is quite prone to errors for maintainers, and the Fedora developer @LecrisUT in an internal discussion proposed the usage of https://cmake.org/cmake/help/v3.30/module/FetchContent.html He will soon write a comment explaining his view. Best regards

metsma commented 5 months ago

Thats why there is code reference in wiki when code changes you can still find relevant parts. How is the fetchcontent better current download solution?

LecrisUT commented 5 months ago

Hi,

FetchContent is the recommended workflow for modern CMake because it allows the maximum flexibility for users, developers, packagers and downstream consumer. While it is designed for integrating with CMake dependencies it can and should be used for downloading arbitrary files. Some features:

Please use this especially for replacing find_package, because there is native support to have fallback from find_package to FetchContent, with built-in options to preffer the latter or only use the former.


Regarding theacomment in the DevelopersTip, I wish that you don't find it offensive, but I agree that such workflow is inappropriate, because for starters the links are pointing to a master blob which will change over time, and the CMake recipe should be well-defined by the project authors to express what is supported and not through usage of options.

There is of course multiple alternative designs besides FetchContent to avoid the workflow in the DevelopersTip, simplest being to control it with options, but because we are not developers of the project, I at least cannot give more detailed suggestions or PR support without a bit of context, knowledge of requirements, or forbidden setups from the project developers. I would happily offer review and advice support on anything CMake related.

metsma commented 5 months ago

My concern is that it should not be offline. The config files should be updated regularly. Least once on every release. https://src.fedoraproject.org/rpms/qdigidoc/blob/rawhide/f/config.json As example 20230504110015Z over year old config is packaged. How can we avoid this? Does the FetchContent solve this somehow? Does it still download the resources online somehow?

LecrisUT commented 5 months ago

Thanks for the explanation. Would it make sense to use a versioning like 4.6.0^YYYYMMDD? On the Fedora packaging side unfortunately all sources must be vetted for licensing issues and uploaded manually (or semi-manually).

FetchContent by default will download the files, so your primary build will not be affected, but on the Fedora side we will have to use FETCHCONTENT_SOURCE_DIR_<uppercaseName> to redirect where this should point to because we do not have internet connection when doing a build.

We can however offer upstream more control of this process to perform this update more regularly. But it depends how regular is this required?

The config files should be updated regularly. Least once on every release.

This at least we can accommodate without any infrastructure change (except for the handling the download section in the code). @Germano0 can you make sure there is a note in the README downstream to remind maintainers to update this file on each release?


However I think having the build dependent on something that changes so regularly is a design flaw. I would recommend three possible approaches (not mutually exclusive):

metsma commented 5 months ago

On the Fedora packaging side unfortunately all sources must be vetted for licensing issues and uploaded manually

I am actually not sure how the config and TSL-s are licensed.

However I think having the build dependent on something that changes so regularly is a design flaw.

This is something that EU designed to distribute digital signature trust chains and there are many moving parts. It is updated by DigiDoc regularly but packaging gives good first offline impression when validating files. https://eidas.ec.europa.eu/efda/tl-browser/#/screen/home

There is already tool for downloading TSL-s https://github.com/open-eid/DigiDoc4-Client/blob/master/client/TSLDownload.cpp

I can probably change the build if files are already in build directory or least in source directory then it will skip the downloading part and continue.

LecrisUT commented 5 months ago

I am actually not sure how the config and TSL-s are licensed.

This would depend on the distribution page, but I don't see anything obvious in the header metadata, and top page (https://id.eesti.ee/) is empty. Ianal, but these could be simply unlicensed. It would be good if someone can confirm these.

I can probably change the build if files are already in build directory or least in source directory then it will skip the downloading part and continue.

Yes, of course that would work as well. FetchContent is more modern and native supported approach for this, which can minimize maintenance, since most features like caching are already baked in.

This is something that EU designed to distribute digital signature trust chains and there are many moving parts. It is updated by DigiDoc regularly but packaging gives good first offline impression when validating files. https://eidas.ec.europa.eu/efda/tl-browser/#/screen/home

Thanks for the reference, I will forward this to see how we can incorporate it with our monitoring backend (https://release-monitoring.org)


What would be very helpful, for designing the CMake project, managing the monitor and for packaging is if all of these files can be aggregated into a single folder (or better yet, and archive). Having a script to check the downloaded/local sources daily and update if contents change should be rather trivial to implement, if not already provided as a service somewhere. I could ask the forum for their experience on such tools if it helps.

LecrisUT commented 5 months ago

Another point that will help is to understand the distinction between the various downloaded files:

URL:            https://github.com/open-eid/DigiDoc4-Client
Source0:        %{url}/releases/download/v%{version}/%{upstream_name}-%{version}.tar.gz
# https://id.eesti.ee/config.json
Source1:        config.json
# https://id.eesti.ee/config.pub
Source2:        config.pub
# https://id.eesti.ee/config.rsa
Source3:        config.rsa
# https://github.com/open-eid/DigiDoc4-Client/wiki/DeveloperTips
Source4:        config.qrc
# https://sr.riik.ee/tsl/estonian-tsl.xml
Source5:        EE.xml
# https://github.com/open-eid/DigiDoc4-Client/wiki/DeveloperTips
Source6:        TSL.qrc
# https://ec.europa.eu/tools/lotl/eu-lotl.xml`
Source7:        eu-lotl.xml

For example, config.json seems rather meaningless to have downloaded. It could be included in the github repo and have the server redirect to the raw file url.

metsma commented 5 months ago

For example, config.json seems rather meaningless to have downloaded. It could be included in the github repo and have the server redirect to the raw file url.

What do you mean?

Only file config.pub that does not change is public key file. config.json, config.rsa change regularly. Cert pinning certificates/version change notification/possible to disable out of support digidoc versions and etc. The TSL files expire in every 6 months + additional changes.

LecrisUT commented 5 months ago

Sorry, I meant config.qrc

metsma commented 5 months ago

Sorry, I meant config.qrc

It is already in repo https://github.com/open-eid/qt-common/blob/master/config.qrc

LecrisUT commented 5 months ago

Sorry, I meant config.qrc

It is already in repo https://github.com/open-eid/qt-common/blob/master/config.qrc

Ok, must have been outdated from 4.4.0.

Then what about the remaining points?

  • Which sources are necessary to update regularly, my guess is only eu-lotl.xml?

  • Which sources are used only at build time

  • If we are to track a file, which one should it be? Is there a way to get a date of when that file is updated? Probably this can work with a plain ftp source.

A more specific breakdown for the files. The idea is that if we make the versioning like 4.6.0^YYYYMMDD to incorporate the date of the certificate updates, which ones are necessary for the user, which ones are necessary only at compile time, and which ones are "must be updated" in order for the program to function. That would help steer how we can add these to release-monitoring.org, and figure out if it's possible to separate the main app binary from the certificates.

metsma commented 5 months ago

I think updating files with DigiDoc release is good enough. config.json, config.rsa, eu-lotl.xml, EE.xml. All the files are also updated regularly on runtime.

LecrisUT commented 5 months ago

All the files are also updated regularly on runtime.

Aaah, perfect to know. I was just over-complicating things. Than we can just add a note to update those files whenever a new release is published. I was afraid that if we were using too old of files, it could brick itself.

Then the only thing that we would need are possibilities to redirect the source of these files. Feel free to implement whatever approach works best for you, and if you need eyes to review, feel free to ping me. The minimum that we would need is to remove the need for manual patching, and to be able to point to the pre-downloaded files.

Here are some snippets that you could use for reference:

Navigating a bit I find that these should be updated upstream at open-eid/qt-common. That should be a standalone (importable) CMake project that is used via FetchContent/find_package, but I degrees.

Regarding add_custom_command(OUTPUT TSL.qrc), it can work with a similar if (EXISTS) guard, but we can also write the file in the build directory before running the cmake --build (assuming TSL.qrc, EE.xml, and eu-lotl.xml are the only artifacts created there). It would be rather problematic if any of the filenames/sturcutre change.

ghost commented 5 months ago

Hello, i would like to say that i am having the same problem. I am trying to package digidoc4-client for gentoo, I got most of the fixing done, its just that one particular command:

/usr/bin/x86_64-pc-linux-gnu-g++ -DBUILD_DATE=\"22.06.2024\" -DBUILD_VER=0 -DCDOC2_GET_URL=\"https://cdoc2-keyserver-get\" -DCDOC2_POST_URL=\"https://cdoc2-keyserver-post\" -DCONFIG_URL=\"https://id.eesti.ee/config.json\" -DMAJOR_VER=4 -DMINOR_VER=5 -DMOBILEID_URL=\"https://dd-mid.ria.ee/mid-api\" -DQT_CORE_LIB -DQT_DEPRECATED_WARNINGS_SINCE=051200 -DQT_GUI_LIB -DQT_NETWORK_LIB -DQT_NO_DEBUG -DQT_PRINTSUPPORT_LIB -DQT_SVG_LIB -DQT_WIDGETS_LIB -DRELEASE_VER=1 -DSMARTID_URL=\"https://dd-sid.ria.ee/v1\" -I/var/tmp/portage/app-crypt/digidoc4-client-4.5.1-r1/work/qdigidoc4-4.5.1_build/client -I/var/tmp/portage/app-crypt/digidoc4-client-4.5.1-r1/work/qdigidoc4-4.5.1/client -I/var/tmp/portage/app-crypt/digidoc4-client-4.5.1-r1/work/qdigidoc4-4.5.1_build/client/qdigidoc4_autogen/include -I/var/tmp/portage/app-crypt/digidoc4-client-4.5.1-r1/work/qdigidoc4-4.5.1 -I/var/tmp/portage/app-crypt/digidoc4-client-4.5.1-r1/work/qdigidoc4-4.5.1/common -isystem /usr/include/qt5 -isystem /usr/include/qt5/QtNetwork -isystem /usr/include/qt5/QtCore -isystem /usr/lib64/qt5/mkspecs/linux-g++ -isystem /usr/include/qt5/QtWidgets -isystem /usr/include/qt5/QtGui -isystem /usr/include/qt5/QtPrintSupport -isystem /usr/include/qt5/QtSvg  -march=native -O2 -pipe -std=gnu++17 -flto=auto -fno-fat-lto-objects -fvisibility=hidden -fvisibility-inlines-hidden -fPIC -MD -MT client/CMakeFiles/qdigidoc4.dir/qdigidoc4_autogen/4UGIYD6VGQ/qrc_images.cpp.o -MF client/CMakeFiles/qdigidoc4.dir/qdigidoc4_autogen/4UGIYD6VGQ/qrc_images.cpp.o.d -o client/CMakeFiles/qdigidoc4.dir/qdigidoc4_autogen/4UGIYD6VGQ/qrc_images.cpp.o -c /var/tmp/portage/app-crypt/digidoc4-client-4.5.1-r1/work/qdigidoc4-4.5.1_build/client/qdigidoc4_autogen/4UGIYD6VGQ/qrc_images.cpp

is failing, i assume that this is because it is trying to connect to the internet and since digidoc4-client has been updated alot, and the wiki page for Sandboxed environments isn't updated, it doesnt include instructions on how to fix this.

Germano0 commented 5 months ago

@pxrb for a better code impagination, use ``` code ```

4.5.1 works because yesterday I managed to build it https://src.fedoraproject.org/rpms/qdigidoc/tree/rawhide Also make sure you have `1251.patch'

ghost commented 5 months ago

Thanks. It finally compiled.

metsma commented 5 months ago

@pxrb for a better code impagination, use code

4.5.1 works because yesterday I managed to build it https://src.fedoraproject.org/rpms/qdigidoc/tree/rawhide Also make sure you have `1251.patch'

Please use Qt6

metsma commented 5 months ago

Does #1277 resolve this issue?

LecrisUT commented 5 months ago

Indeed, that's an acceptable minimal solution :+1:. But don't forget to update the wiki page to reflect the difference, and where the pre-downloaded resource files should point to (/ or /common)