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

[Docs] Crypto signatures manuals and version script #1788

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

woju
Copy link
Member

@woju woju commented Feb 27, 2024

How to test this PR?

Will really be tested during new release...


This change is Reviewable

Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @woju)


Documentation/devel/maintainer-manual.rst line 20 at r1 (raw file):

https://github.com/gramineproject/gramine/issues/new?title=Release+%3Cversion%3E+TODO&body=-+%5B+%5D+draft+release+notes+%28%40%3Cowner%3E%29%0A-+%5B+%5D+draft+blogpost+%28%40%3Cowner%3E%29%0A-+%5B+%5D+draft+%23community+announcement+%28%40%3Cowner%3E%29%0A-+%5B+%5D+update+installation+instructions+%28if+a+distro+was+released+since+last+release%29+%28%40%3Cowner%3E%29%0A-+%5B+%5D+create+release+PR+%28%40%3Cowner%3E%29%0A%0Aiterate+%28update+version%2C+build+and+upload+unstable+packages%29%0A%0Afinal+stretch%3A%0A-+%5B+%5D+get+QA+signoff+%28%40%3Cowner%3E%29%0A-+%5B+%5D+approve+PR+%28%40%3Cowner%3E%29%0A-+%5B+%5D+update+version+to+final+and+push+commits+%28%40%3Cowner%3E%29%0A-+%5B+%5D+build+final+packages+%28%40%3Cowner%3E%29%0A-+%5B+%5D+upload+packages+to+release+notes+%28%40%3Cowner%3E%29%0A-+%5B+%5D+push+tag+%28%40%3Cowner%3E%29%0A-+%5B+%5D+switch+release+notes+to+pushed+tag+%28%40%3Cowner%3E%29%0A-+%5B+%5D+merge+PR+%28%40%3Cowner%3E%29%0A-+%5B+%5D+publish+release+notes+%28%40%3Cowner%3E%29%0A-+%5B+%5D+publish+blogpost+%28%40%3Cowner%3E%29%0A-+%5B+%5D+publish+on+%23community+%28%40%3Cowner%3E%29%0A

create PR

Please start titles with capital letter (because you do it in other places here)


Documentation/devel/maintainer-manual.rst line 32 at r1 (raw file):

Then set the PR on reviewable.io to be reviewed commit-by-commit.

update version in PR

ditto


Documentation/devel/maintainer-manual.rst line 37 at r1 (raw file):

.. code-block::

    git reset --hard HEAD~``

What is this syntax with double-backtick?


Documentation/devel/maintainer-manual.rst line 38 at r1 (raw file):

    git reset --hard HEAD~``
    scripts/release.sh X.Y~rcN``

ditto


Documentation/devel/maintainer-manual.rst line 41 at r1 (raw file):

    git push --force

tag

ditto


Documentation/devel/maintainer-manual.rst line 65 at r1 (raw file):

    “Code signing” refers to the process of cryptographically siging your
    contributions (commits and tags), so other people are able to mathematically
    prove that the contribution came from the holder of particular cryptographic

holder of a particular (add the a)


Documentation/devel/maintainer-manual.rst line 71 at r1 (raw file):

Generating key
^^^^^^^^^^^^^^
First, you need to generate you key using :program:`gpg`. The key need to be "sign

key need -> key needs


Documentation/devel/maintainer-manual.rst line 73 at r1 (raw file):

First, you need to generate you key using :program:`gpg`. The key need to be "sign
only"! Otherwise, if you also add encrypt capability, people will add your key
to their MUAs and will encrypt e-mail messages to you using code signing key.

What are MUAs? Please decipher. Mail User Agent?


scripts/release.sh line 78 at r1 (raw file):

"Bump "*)    commit "$V" --amend ;;
*)                  commit "$V" ;;
esac

I don't understand, why do you need this case? Why not just bump $V; commit $V?

Copy link
Member Author

@woju woju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 2 of 4 files reviewed, 9 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @dimakuv)


Documentation/devel/maintainer-manual.rst line 20 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please start titles with capital letter (because you do it in other places here)

Done.


Documentation/devel/maintainer-manual.rst line 32 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

ditto

Done.


Documentation/devel/maintainer-manual.rst line 37 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

What is this syntax with double-backtick?

It's a leftover. Removed.


Documentation/devel/maintainer-manual.rst line 38 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

ditto

Done.


Documentation/devel/maintainer-manual.rst line 41 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

ditto

Done.


Documentation/devel/maintainer-manual.rst line 65 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

holder of a particular (add the a)

Done.


Documentation/devel/maintainer-manual.rst line 71 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

key need -> key needs

Done.


Documentation/devel/maintainer-manual.rst line 73 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

What are MUAs? Please decipher. Mail User Agent?

Done.


scripts/release.sh line 78 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I don't understand, why do you need this case? Why not just bump $V; commit $V?

For release we're creating two commits: the first one changes to version from X.{Y-1} to X.Y~rcZ, and the second one changes to X.Ypost~UNRELEASED. So if you're changing the commit stack from ~rcZ to ~rc{Z+1} (or to final X.Y, doesn't matter for this argument), there are two options:

  • roll back two commits and recreate both of them (git reset HEAD~2 && sed && git commit && sed && git commit);
  • roll back one commit, amend the first one and recreate the second one (git reset HEAD~ && sed && git commit --amend && sed && git commit).

The problem is, if you're updating from ~rcZ to ~rc{Z+1}, the first commit also probably contains non-trivial changes to debian/changelog, which can't be recreated, so --amend is the obvious solution, and that's what manual and the comment suggests. But if you're doing this commit stack for the first time, you need to create both commits anyway and you're not rolling back anything. So, you need to have both code paths available.

The difference is between them is detected based on title of the commit on the HEAD of the branch. If it starts with Bump , then we've most likely just reset the second commit and we're in the middle of the stack, so we just --amend. If not Bump , then we're creating the stack for the first time.

Do you want to have all of this in a comment?

I've fixed whitespace. Previously it said "Bump Gramine "*, but I removed Gramine to make it applicable to all repos (not just gramineproject/gramine, but also gramineproject/contrib etc.), but failed to adjust spaces.

Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed all commit messages.
Reviewable status: 2 of 4 files reviewed, 13 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @dimakuv and @woju)

a discussion (no related file):
Can we wait with this until #1784 is merged? So I could test it on the new Sphinx before merging.



Documentation/devel/maintainer-manual.rst line 70 at r2 (raw file):

Generating key
^^^^^^^^^^^^^^

Please add en empty line here.


Documentation/devel/DCO/index.rst line 6 at r2 (raw file):

    For cryptographical “code signing”, as opposed to “signing off” your
    commits, please refer to :doc:`../maintainer-manual`.

Could you link to the specific section there?


scripts/release.sh line 3 at r2 (raw file):

#!/bin/sh
# SPDX-License-Identifier: LGPL-3.0-or-later
# SPDX-FileCopyrightText: 2024 Wojtek Porczyk <woju@invisiblethingslab.com>

We don't use this format in other files. Also, the copyright should be Intel?

dimakuv
dimakuv previously approved these changes Mar 4, 2024
Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @woju)


scripts/release.sh line 78 at r1 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

For release we're creating two commits: the first one changes to version from X.{Y-1} to X.Y~rcZ, and the second one changes to X.Ypost~UNRELEASED. So if you're changing the commit stack from ~rcZ to ~rc{Z+1} (or to final X.Y, doesn't matter for this argument), there are two options:

  • roll back two commits and recreate both of them (git reset HEAD~2 && sed && git commit && sed && git commit);
  • roll back one commit, amend the first one and recreate the second one (git reset HEAD~ && sed && git commit --amend && sed && git commit).

The problem is, if you're updating from ~rcZ to ~rc{Z+1}, the first commit also probably contains non-trivial changes to debian/changelog, which can't be recreated, so --amend is the obvious solution, and that's what manual and the comment suggests. But if you're doing this commit stack for the first time, you need to create both commits anyway and you're not rolling back anything. So, you need to have both code paths available.

The difference is between them is detected based on title of the commit on the HEAD of the branch. If it starts with Bump , then we've most likely just reset the second commit and we're in the middle of the stack, so we just --amend. If not Bump , then we're creating the stack for the first time.

Do you want to have all of this in a comment?

I've fixed whitespace. Previously it said "Bump Gramine "*, but I removed Gramine to make it applicable to all repos (not just gramineproject/gramine, but also gramineproject/contrib etc.), but failed to adjust spaces.

I'm fine with this explanation here. For historical digging, if ever needed, we'll get to your comment on GitHub.

@woju woju changed the title [Docs] Add maintainer's manual and version script [Docs] Crypto signatures manuals and version script Apr 22, 2024
Copy link
Member Author

@woju woju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 9 files reviewed, 4 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @dimakuv and @mkow)

a discussion (no related file):

Previously, mkow (Michał Kowalczyk) wrote…

Can we wait with this until #1784 is merged? So I could test it on the new Sphinx before merging.

It's merged. Do you want a rebase?



Documentation/devel/maintainer-manual.rst line 70 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Please add en empty line here.

Done.


Documentation/devel/DCO/index.rst line 6 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Could you link to the specific section there?

Done.


scripts/release.sh line 3 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

We don't use this format in other files. Also, the copyright should be Intel?

IDK about header, I found it somewhere, maybe we should use it project wide? For now I've reverted to the version without header for consistency.

About copyright, I think this script might have been done on scaffolding time this year. Probably not Intel.

@woju woju force-pushed the woju/docs-202402 branch 2 times, most recently from 907b069 to 7057718 Compare April 25, 2024 09:27
Copy link
Member Author

@woju woju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 10 files reviewed, 5 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @dimakuv and @mkow)


Documentation/devel/maintainer-manual.rst line 9 at r3 (raw file):

Create new checklist issue (fill all ``<variable>`` before submitting):

.. new-issue:: Release <version> checklist

I'm sorry for this custom directive, but I got fed up with manual editing of the query string.

@mkow mkow requested a review from dimakuv April 28, 2024 15:44
Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 10 files reviewed, 5 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @dimakuv and @woju)

a discussion (no related file):

Previously, woju (Wojtek Porczyk) wrote…

It's merged. Do you want a rebase?

Yes, please.


Copy link
Member Author

@woju woju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 10 files reviewed, 5 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @dimakuv and @mkow)

a discussion (no related file):

Previously, mkow (Michał Kowalczyk) wrote…

Yes, please.

It already is rebased. I think I rebased it when writing this second commit about verifying signatures.


Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 8 of 10 files at r3, all commit messages.
Reviewable status: 8 of 10 files reviewed, 5 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @mkow)

a discussion (no related file):

Previously, woju (Wojtek Porczyk) wrote…

It already is rebased. I think I rebased it when writing this second commit about verifying signatures.

I enabled this PR in ReadTheDocs: https://gramine.readthedocs.io/en/woju-docs-202402/


Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 10 files at r3.
Reviewable status: all files reviewed, 12 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @mkow and @woju)


Documentation/verify-sig.rst line 115 at r3 (raw file):

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

If you happen to have your own PGP key pair, you can choose sign the key with

choose sign -> choose to sign


Documentation/verify-sig.rst line 178 at r3 (raw file):

.. no, I don't have "0000000000000000" key

If you know what you're doing, you can use another singing command in place of

singing -> signing


Documentation/verify-sig.rst line 206 at r3 (raw file):

    gpg:                using EDDSA key 9C4D27D9157EF771A4283926044D9664E7A77E16
    gpg: Good signature from "Wojciech Porczyk (Gramine code signing key) <woju@invisiblethingslab.com>" [full]
    % git verify-tag v1.6.2

Could you add an empty line before this command?


Documentation/verify-sig.rst line 254 at r3 (raw file):

    [...]
    
    % % git verify-commit a971e30f3430b4b8079ec42f5d035ced68130bdc

% % -> %


Documentation/devel/maintainer-manual.rst line 80 at r3 (raw file):

    signature. For details, please read :doc:`DCO/index`.

    “Code signing” refers to the process of cryptographically siging your

siging -> signing


Documentation/devel/maintainer-manual.rst line 89 at r3 (raw file):

^^^^^^^^^^^^^^

First, you need to generate you key using :program:`gpg`. The key needs to be

you key -> your key


Documentation/devel/maintainer-manual.rst line 94 at r3 (raw file):

to you using code signing key. This is not desired, the key generated for the
purpose of code signing should not be used in any other context (e.g. e-mail or
singing code in other projects).

singing -> signing

Copy link
Member Author

@woju woju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 8 of 10 files reviewed, 12 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @dimakuv and @mkow)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I enabled this PR in ReadTheDocs: https://gramine.readthedocs.io/en/woju-docs-202402/

Adding separate versions is not needed anymore, since we build all PRs: https://gramine--1788.org.readthedocs.build/en/1788/ (you can substitute any valid number of a PR).

Webhook works, just reporting the status doesn't.



Documentation/verify-sig.rst line 115 at r3 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

choose sign -> choose to sign

Done.


Documentation/verify-sig.rst line 178 at r3 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

singing -> signing

Done.


Documentation/verify-sig.rst line 206 at r3 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Could you add an empty line before this command?

Just empty line, no, I don't want to, because it is (was) a single listing. I guess I can split it into two paragraphs instead. Did the same to verifying commits too.


Documentation/verify-sig.rst line 254 at r3 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

% % -> %

Done.


Documentation/devel/maintainer-manual.rst line 80 at r3 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

siging -> signing

Done.


Documentation/devel/maintainer-manual.rst line 89 at r3 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

you key -> your key

Done.


Documentation/devel/maintainer-manual.rst line 94 at r3 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

singing -> signing

Done.

dimakuv
dimakuv previously approved these changes May 8, 2024
Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @mkow)

a discussion (no related file):

Previously, woju (Wojtek Porczyk) wrote…

Adding separate versions is not needed anymore, since we build all PRs: https://gramine--1788.org.readthedocs.build/en/1788/ (you can substitute any valid number of a PR).

Webhook works, just reporting the status doesn't.

Ah, awesome.


Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 8 of 10 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @woju)


-- commits line 7 at r4:
TODO for myself: verify commit split before the rebase.


Documentation/index.rst line 218 at r4 (raw file):

   devel/onboarding
   devel/setup
   devel/maintainer-manual

I'd push this down on the list, it's important only to very few people and will be read much less often than the other entries here.


Documentation/verify-sig.rst line 58 at r4 (raw file):

Then check the key fingerprint. If you're sure the key is correct, you need to
mark it as trusted.

Suggestion:

Then check the key fingerprint. After ensuring the key is correct, you can
mark it as trusted.

Documentation/devel/maintainer-manual.rst line 1 at r4 (raw file):

Maintainer's manual

I think this is rather a "Release maintainer's manual"?


Documentation/devel/maintainer-manual.rst line 35 at r4 (raw file):

    - [ ] publish on #community (@<owner>)

Create PR

Suggestion:

Create a PR

Documentation/devel/maintainer-manual.rst line 47 at r4 (raw file):

Then set the PR on reviewable.io to be reviewed commit-by-commit.

Update version in PR

Suggestion:

Update version in the PR

Documentation/devel/maintainer-manual.rst line 74 at r4 (raw file):
The meaning of it depends on the project, quoting git docs:

The meaning of a signoff depends on the project to which you’re committing.

I'd also change the last sentence here accordingly.

Suggestion:

    “Signing off” is a |~| legal device for a |~| sort of signature by which you
    usually assert that you are holding copyrights to the code you're submitting (or

Documentation/devel/maintainer-manual.rst line 91 at r4 (raw file):

First, you need to generate your own key pair using :program:`gpg`. The key
needs to be "sign only"! Otherwise, if you also add encrypt capability, people
will add your key to their :abbr:`MUA (Mail User Agent)`\ s and will encrypt

I think this term is much more popular nowadays and thus doesn't need an explanation.

Suggestion:

email clients

Documentation/devel/maintainer-manual.rst line 192 at r4 (raw file):

    git config --global gpg.program qubes-gpg-client-wrapper

and remember to set ``QUBES_GPG_DOMAIN`` envrionment variable in your shell

Suggestion:

environment

scripts/release.sh line 3 at r2 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

IDK about header, I found it somewhere, maybe we should use it project wide? For now I've reverted to the version without header for consistency.

About copyright, I think this script might have been done on scaffolding time this year. Probably not Intel.

Then it should be ITL?

Copy link
Member Author

@woju woju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 7 of 10 files reviewed, 10 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @dimakuv, @mkow, and @woju)


Documentation/index.rst line 218 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I'd push this down on the list, it's important only to very few people and will be read much less often than the other entries here.

Hmm, right, it certainly should be below coding-style. I moved it to the bottom of technical ones, leaving only legal stuff below.


Documentation/verify-sig.rst line 58 at r4 (raw file):

Then check the key fingerprint. If you're sure the key is correct, you need to
mark it as trusted.

Done.


Documentation/devel/maintainer-manual.rst line 1 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I think this is rather a "Release maintainer's manual"?

No, it also describes code signing which I hope will be used by all maintainers (maybe even all contributors), not just me. Also we don't have a position of "release maintainer" or "release manager", all maintainers can (eventually should be able to) do a release (i.e. sign a tag).

If you want it to be release manual, I can move the section about generating keys and configuring git to devel/setup.rst (right now it's only about configuring vim).


Documentation/devel/maintainer-manual.rst line 35 at r4 (raw file):

    - [ ] publish on #community (@<owner>)

Create PR

Done.


Documentation/devel/maintainer-manual.rst line 47 at r4 (raw file):

Then set the PR on reviewable.io to be reviewed commit-by-commit.

Update version in PR

Done.

(I also rewrote next heading to "Create a tag" to match all previous headings).


Documentation/devel/maintainer-manual.rst line 74 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

The meaning of it depends on the project, quoting git docs:

The meaning of a signoff depends on the project to which you’re committing.

I'd also change the last sentence here accordingly.

I think it should be as written, I describe what signing off means in our project. I can add "(in our project)" in some strategic place, or add a sentence in parens like >(In other projects "singing off" might have slightly different meaning, check the details with the respective project every time.)


Documentation/devel/maintainer-manual.rst line 91 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I think this term is much more popular nowadays and thus doesn't need an explanation.

There are different "email clients" doing different stuff, so I wanted to optimise the phraseology for precison. This is manual for maintainers, there's no need to dumb it down.


Documentation/devel/maintainer-manual.rst line 192 at r4 (raw file):

    git config --global gpg.program qubes-gpg-client-wrapper

and remember to set ``QUBES_GPG_DOMAIN`` envrionment variable in your shell

Done.


scripts/release.sh line 3 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Then it should be ITL?

No, in ITL we don't reassign copyrights (see Qubes repos).

Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @woju)


Documentation/devel/maintainer-manual.rst line 1 at r4 (raw file):

No, it also describes code signing which I hope will be used by all maintainers

I agree with that, but this document is more specific, and the title should describe its contents well - "Maintainer's manual" sounds like a very broad document about everything relating to maintaining Gramine. And at the same time, when looking at ToC you won't easily find where exactly packaging/releasing procedures are described.

So - describe the contents, not the intended reader group.


Documentation/devel/maintainer-manual.rst line 74 at r4 (raw file):

I describe what signing off means in our project

That's my problem with the current version, nowhere it says that it's only about how it works in our project. Currently it sounds to me like a generic explanation of this git feature: "“Signing off” is a legal device [...] by which you assert that you are holding copyrights".

I can add "(in our project)" in some strategic place, or add a sentence in parens like >(In other projects "singing off" might have slightly different meaning, check the details with the respective project every time.)

+1, both do the work IMO


Documentation/devel/maintainer-manual.rst line 91 at r4 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

There are different "email clients" doing different stuff, so I wanted to optimise the phraseology for precison. This is manual for maintainers, there's no need to dumb it down.

Maybe I'm missing something, but what's the difference between MUA and email client?

Copy link
Member Author

@woju woju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 9 of 10 files reviewed, 5 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @mkow)


Documentation/devel/maintainer-manual.rst line 1 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

No, it also describes code signing which I hope will be used by all maintainers

I agree with that, but this document is more specific, and the title should describe its contents well - "Maintainer's manual" sounds like a very broad document about everything relating to maintaining Gramine. And at the same time, when looking at ToC you won't easily find where exactly packaging/releasing procedures are described.

So - describe the contents, not the intended reader group.

"Code signing and release manual"? Because those 2 topics are very different, they're connected just by the fact they're intended for maintainers, so I don't thing "Maintainer's manual" is wrong.

Or I can split those in 2 documents, 'devel/release.rstand put keygen intodevel/setup.rst`.


Documentation/devel/maintainer-manual.rst line 74 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I describe what signing off means in our project

That's my problem with the current version, nowhere it says that it's only about how it works in our project. Currently it sounds to me like a generic explanation of this git feature: "“Signing off” is a legal device [...] by which you assert that you are holding copyrights".

I can add "(in our project)" in some strategic place, or add a sentence in parens like >(In other projects "singing off" might have slightly different meaning, check the details with the respective project every time.)

+1, both do the work IMO

Done.


Documentation/devel/maintainer-manual.rst line 91 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Maybe I'm missing something, but what's the difference between MUA and email client?

There are several kinds of email software that technically are clients (in the "client-server architecture" sense): MTA (Mail Transport Agent) is also a client of SMTP protocol, but it performs different role: forwards email to "server", which may be another MTA or MDA (... Delivery Agent). There are MRAs (... Retrieval Agents), which download email from MDAs, but can't show it to people, so they're closer to MTA, just with reversed polarity (they're clients of IMAP and POP3 protocols instead of SMTP). Email packages typically package several of those roles into one more or less integrated solution (Thunderbird is MUA+MRA+limited MTA capabilities, Postfix is MSA+MTA+limited MDA, and so on). For this reason email people don't like to use unqualified "client" (or "server"), because it's immediately met with "yeah, which client?".

https://en.wikipedia.org/wiki/Email_agent_(infrastructure),
https://en.wikipedia.org/wiki/Simple_Mail_Transfer_Protocol#Mail_processing_model, and diagram in https://en.wikipedia.org/wiki/Message_delivery_agent

Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @dimakuv, @kailun-qin, and @woju)


Documentation/devel/maintainer-manual.rst line 1 at r4 (raw file):

"Code signing and release manual"

Sounds good to me. We use code signing only for the release tags, so it's not a bad idea to be placed in the very same document.


Documentation/devel/maintainer-manual.rst line 91 at r4 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

There are several kinds of email software that technically are clients (in the "client-server architecture" sense): MTA (Mail Transport Agent) is also a client of SMTP protocol, but it performs different role: forwards email to "server", which may be another MTA or MDA (... Delivery Agent). There are MRAs (... Retrieval Agents), which download email from MDAs, but can't show it to people, so they're closer to MTA, just with reversed polarity (they're clients of IMAP and POP3 protocols instead of SMTP). Email packages typically package several of those roles into one more or less integrated solution (Thunderbird is MUA+MRA+limited MTA capabilities, Postfix is MSA+MTA+limited MDA, and so on). For this reason email people don't like to use unqualified "client" (or "server"), because it's immediately met with "yeah, which client?".

https://en.wikipedia.org/wiki/Email_agent_(infrastructure),
https://en.wikipedia.org/wiki/Simple_Mail_Transfer_Protocol#Mail_processing_model, and diagram in https://en.wikipedia.org/wiki/Message_delivery_agent

This differentiation seem to be super specific to email client developers. Also, even the links you pasted seem to say that "(e)mail client" == "MUA":

I think "MUA" is a rather confusing and not a well-known term, while "email client" is clear and doesn't require any additional explanations to be understood.

@kailun-qin, @dimakuv: any preferences?

dimakuv
dimakuv previously approved these changes Jun 3, 2024
Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @kailun-qin, @mkow, and @woju)


Documentation/devel/maintainer-manual.rst line 1 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

"Code signing and release manual"

Sounds good to me. We use code signing only for the release tags, so it's not a bad idea to be placed in the very same document.

I also like a long name "Code signing and release manual"


Documentation/devel/maintainer-manual.rst line 91 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

This differentiation seem to be super specific to email client developers. Also, even the links you pasted seem to say that "(e)mail client" == "MUA":

I think "MUA" is a rather confusing and not a well-known term, while "email client" is clear and doesn't require any additional explanations to be understood.

@kailun-qin, @dimakuv: any preferences?

I am fine with MUA. For example, I periodically use Mutt as a MUA (i.e., not for receiving/sending emails, but purely for constructing them), so I'm aware of the terminology and it doesn't put me off.

Copy link
Member Author

@woju woju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 8 of 11 files reviewed, 4 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @kailun-qin and @mkow)


Documentation/devel/maintainer-manual.rst line 1 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I also like a long name "Code signing and release manual"

Done, and swapped code signing and release to match title, and because code signing will potentially be useful to more people.

Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 3 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @woju)


-- commits line 7 at r4:

Previously, mkow (Michał Kowalczyk) wrote…

TODO for myself: verify commit split before the rebase.

s/before/after/

dimakuv
dimakuv previously approved these changes Jun 5, 2024
Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 3 files at r5, 2 of 3 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @woju)

Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 7 of 10 files at r3, 1 of 3 files at r5, 1 of 3 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @mkow and @woju)


Documentation/verify-sig.rst line 63 at r8 (raw file):

^^^^^^^^^^^^^^^^^^^^^^^^^

If you don't have you own PGP key pair, you can mark the key as ultimately

-> your

Code quote:

you

Documentation/verify-sig.rst line 123 at r8 (raw file):

    This is free software: you are free to change and redistribute it.
    There is NO WARRANTY, to the extent permitted by law.
    

What are these spaces in this file?

Code quote:

···

Documentation/verify-sig.rst line 232 at r8 (raw file):

    Primary key fingerprint: 9C4D 27D9 157E F771 A428  3926 044D 9664 E7A7 7E16

Which is **NOT a successful verification**, because the key might be

-> must?

Code quote:

might

Documentation/devel/code-signing-and-releasing.rst line 41 at r8 (raw file):

code signing in this project.

The key needs to be RSA (at least 3072 to match overall security level in SGX)

Maybe "the key needs to offer a security strength of 128 bits"? ("the key needs to be RSA or Curve25519" reads a bit weird to me, RSA is the algo and Curve25519 is actually the curve)

Code quote:

The key needs to be RSA (at least 3072 to match overall security level in SGX)

Copy link
Member Author

@woju woju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 10 of 11 files reviewed, 6 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @dimakuv, @kailun-qin, and @mkow)


Documentation/verify-sig.rst line 63 at r8 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

-> your

Done.


Documentation/verify-sig.rst line 123 at r8 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

What are these spaces in this file?

They are part of the indentation of the whole listing, are done when I indent all (so that every line of the listing can just be dedented by 4 spaces, no exceptions). RST actually does not make them mandatory (in contrast to some other markups), so I can remove them if you want.


Documentation/verify-sig.rst line 232 at r8 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

-> must?

No, as exemplified by the listing: this is actually the correct public key (see fingerprint), but without signature and without trust bit. So it may or may not be different, the gpg tool just isn't sure.


Documentation/devel/code-signing-and-releasing.rst line 41 at r8 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

Maybe "the key needs to offer a security strength of 128 bits"? ("the key needs to be RSA or Curve25519" reads a bit weird to me, RSA is the algo and Curve25519 is actually the curve)

I don't want to explain what is strength as expressed in bits and how to recalculate RSA's bit length to that theoretical strength. Recommendation of 3072 bits in RSA is mostly arbitrary and an appeal to authority, because again, I don't want to discuss bit lengths, so I defer to SGX strength (the underlying threat model is that the target is still user's data, but now the attacker wants to subvert the supply chain by forging signatures, so the signature does not need to be stronger than 3072, otherwise attacker will just attack the signature in SIGSTRUCT).

"Curve25519" specifies not only the curve, but also the field and base point, and implementations take advantage of mathematical properties that are not generally available for other curves, so it's closer to "algorithm" than to just bit length. The algorithm is "EdDSA", and the actual curve is "Ed25519", which is mathematically equivalent to Curve25519, but technically not the same (https://en.wikipedia.org/wiki/EdDSA#Ed25519). Again, I don't want to explain all of that.

"RSA" and "Curve25519" are choices available in the --full-gen-key menu (the menu item is with space, but most people talk about that scheme without space, https://en.wikipedia.org/wiki/Curve25519, and I think it's close enough). Notice the wording in GPG questionnaire that is technically correct: first "ECC", which is correct, this is elliptic key, and then the question "[...] which elliptic curve" is answered by "Curve25519", which is mathematically equivalent to the actual Ed25519 curve. I think they did that because of "ECC and ECC" option (sign and encrypt), where there are actually two curves, but closely related. This can be compared to RSA option, which also does not permit to generate sign and encrypt keypairs with different lengths.

So IDK, I probably can reword to say "RSA (at least 3072 bit [...]) or an elliptic key on Curve25519.", but I don't want to mention 128 bit strength, nor further mention "Edwards" or "Ed25519" or something like that, because developers who are not cryptographers might start looking for that option in the menu and won't find them. This is not a crypto handbook.

Copy link
Member Author

@woju woju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 10 of 11 files reviewed, 6 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @dimakuv, @kailun-qin, and @mkow)


Documentation/devel/code-signing-and-releasing.rst line 41 at r8 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

I don't want to explain what is strength as expressed in bits and how to recalculate RSA's bit length to that theoretical strength. Recommendation of 3072 bits in RSA is mostly arbitrary and an appeal to authority, because again, I don't want to discuss bit lengths, so I defer to SGX strength (the underlying threat model is that the target is still user's data, but now the attacker wants to subvert the supply chain by forging signatures, so the signature does not need to be stronger than 3072, otherwise attacker will just attack the signature in SIGSTRUCT).

"Curve25519" specifies not only the curve, but also the field and base point, and implementations take advantage of mathematical properties that are not generally available for other curves, so it's closer to "algorithm" than to just bit length. The algorithm is "EdDSA", and the actual curve is "Ed25519", which is mathematically equivalent to Curve25519, but technically not the same (https://en.wikipedia.org/wiki/EdDSA#Ed25519). Again, I don't want to explain all of that.

"RSA" and "Curve25519" are choices available in the --full-gen-key menu (the menu item is with space, but most people talk about that scheme without space, https://en.wikipedia.org/wiki/Curve25519, and I think it's close enough). Notice the wording in GPG questionnaire that is technically correct: first "ECC", which is correct, this is elliptic key, and then the question "[...] which elliptic curve" is answered by "Curve25519", which is mathematically equivalent to the actual Ed25519 curve. I think they did that because of "ECC and ECC" option (sign and encrypt), where there are actually two curves, but closely related. This can be compared to RSA option, which also does not permit to generate sign and encrypt keypairs with different lengths.

So IDK, I probably can reword to say "RSA (at least 3072 bit [...]) or an elliptic key on Curve25519.", but I don't want to mention 128 bit strength, nor further mention "Edwards" or "Ed25519" or something like that, because developers who are not cryptographers might start looking for that option in the menu and won't find them. This is not a crypto handbook.

or maybe "elliptic Ed25519 key (called Curve25519 in GPG)"? not sure at this point

Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @mkow and @woju)


Documentation/verify-sig.rst line 123 at r8 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

They are part of the indentation of the whole listing, are done when I indent all (so that every line of the listing can just be dedented by 4 spaces, no exceptions). RST actually does not make them mandatory (in contrast to some other markups), so I can remove them if you want.

Yeah, I prefer removing them, also some other trailing spaces (e.g., after usage: SC below)


Documentation/verify-sig.rst line 232 at r8 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

No, as exemplified by the listing: this is actually the correct public key (see fingerprint), but without signature and without trust bit. So it may or may not be different, the gpg tool just isn't sure.

Right, I misread. Thanks for the explanation.


Documentation/devel/code-signing-and-releasing.rst line 41 at r8 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

or maybe "elliptic Ed25519 key (called Curve25519 in GPG)"? not sure at this point

I see, let's not involve strength and other details then -- let's keep the wording as is.

Copy link
Member Author

@woju woju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @kailun-qin, @mkow, and @woju)


Documentation/verify-sig.rst line 123 at r8 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

Yeah, I prefer removing them, also some other trailing spaces (e.g., after usage: SC below)

Done. I hope no-one will be pasting this into expect scripts.

Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mkow and @woju)

Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @woju)

Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @mkow and @woju)


Documentation/devel/code-signing-and-releasing.rst line 142 at r10 (raw file):

Create new checklist issue (fill all ``<variable>`` before submitting):

.. new-issue:: Release <version> checklist

can we add "update docker image" to the checklist (so that we won't forget about that)?

Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @kailun-qin, @mkow, and @woju)


Documentation/devel/code-signing-and-releasing.rst line 142 at r10 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

can we add "update docker image" to the checklist (so that we won't forget about that)?

+1

woju added 2 commits June 12, 2024 12:10
Signed-off-by: Wojtek Porczyk <woju@invisiblethingslab.com>
Signed-off-by: Wojtek Porczyk <woju@invisiblethingslab.com>
Copy link
Member Author

@woju woju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 9 of 11 files reviewed, 3 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @dimakuv, @kailun-qin, and @mkow)


Documentation/devel/code-signing-and-releasing.rst line 142 at r10 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

+1

Done. Took liberty to add a pattern for release notes and an item there to mention distro in docker image.

Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r11, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @kailun-qin, @mkow, and @woju)

Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r11, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mkow and @woju)

Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r11, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @woju)

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.

4 participants