[wotmate] Tweak trust path formatting for readability

Commit Message

Konstantin Ryabitsev March 26, 2025, 3:37 p.m. UTC

Tweak the trust paths output:

- Add a '# Trust paths' header to the paths section
- Show the depth of the signature
- Don't use '.' to represent the target (I'm the one who suggested it,
  but it doesn't make sense with these formatting changes)
- Only modify the key for trust path changes if it already has trust
  path info in it, to avoid having all keys change at once.

Signed-off-by: Konstantin Ryabitsev <[e-mail]>
---
Cc: Uwe Kleine-König <[e-mail]>
---
 export-keyring.py | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)


---
base-commit: f4624640df1c3113e7122f8f160b7a86cbba80b9
change-id: 20250326-trust-paths-improvements-965b9971bbe4
prerequisite-message-id: <[e-mail]>
prerequisite-patch-id: 7868b2463a94c294b1406727343bd405fadbacd1

Best regards,

Comments

Uwe Kleine-König March 26, 2025, 6:04 p.m. UTC | #1

On Wed, Mar 26, 2025 at 11:37:15AM -0400, Konstantin Ryabitsev wrote:
> Tweak the trust paths output:
> 
> - Add a '# Trust paths' header to the paths section
> - Show the depth of the signature
> - Don't use '.' to represent the target (I'm the one who suggested it,
>   but it doesn't make sense with these formatting changes)
> - Only modify the key for trust path changes if it already has trust
>   path info in it, to avoid having all keys change at once.
> 
> Signed-off-by: Konstantin Ryabitsev <[e-mail]>
> ---
> Cc: Uwe Kleine-König <[e-mail]>
> ---
>  export-keyring.py | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/export-keyring.py b/export-keyring.py
> index 3e455f9..967e1ef 100755
> --- a/export-keyring.py
> +++ b/export-keyring.py
> @@ -110,22 +110,34 @@ if __name__ == '__main__':
>              logger.debug('Skipping %s due to invalid WoT', kid)
>              continue
>  
> -        key_paths_repr = '\n'.join('\n `-> '.join(wotmate.get_uiddata_by_pubrow(c, rowid) if rowid != to_rowid else "." for rowid in kp) for kp in key_paths).encode('utf-8')
> +        kpblock = ''
> +        for kp in key_paths:
> +            for lvl, rowid in enumerate(kp):
> +                kpuid = wotmate.get_uiddata_by_pubrow(c, rowid)
> +                kpblock += f'   {lvl}  {kpuid}' if lvl > 0 else f'from  {kpuid}'
> +                kpblock += '\n'
> +            kpblock += '\n'

Now this isn't a one-liner any more you can also work with indention as
you proposed initially. Something like

                kpblock += f'{4 * lvl * " "}`-> {kpuid}' if lvl > 0 else f'{kpuid}'

(untested).

> +        key_paths_repr = kpblock.encode()

Did you drop 'utf-8' here on purpose? I think it's sensible to keep as
the output of gpg --with-colons is expected to be in utf-8 (though I
don't find that in the docs immediately) and you don't want all graphs
regenerated just because the caller's locale differs from the previous
caller's.

>  
>          # Do we already have a file in place?
>          if os.path.exists(keyout):
>              # Load it up and see if it's different
>              with open(keyout, 'rb') as fin:
>                  old_keyexport = fin.read()
> -                if old_keyexport.find(keydata) > 0 and old_keyexport.find(key_paths_repr) > 0:
> -                    logger.debug('No changes for %s', kid)
> -                    continue
> +                if keydata in old_keyexport:
> +                    logger.debug('No key changes for %s', kid)
> +                    if b'\n# Trust paths\n' not in old_keyexport:
> +                        logger.debug('Old key does not include trust paths, skipping')
> +                        continue
> +                    if key_paths_repr in old_keyexport:
> +                        logger.debug('No trust path changes for %s', kid)
> +                        continue

I would add the trust path info already now in a commit changing all
keys at once. Then we can benefit from it on the next real change of the
data.

I never was in the situation to have to justify an "why did all the keys
change" situation, but without knowing how that was for you in the past,
I wouldn't fear that. (Assuming that it's only the trust path that is
added and keydata doesn't actually change.)

>          # Now, export the header
>          args = ['--list-options', 'show-notations', '--list-options',
>                  'no-show-uid-validity', '--with-subkey-fingerprints', '--list-key', kid]
>          header = wotmate.gpg_run_command(args, with_colons=False)
> -        keyexport = header + b'\n\n' + key_paths_repr + b'\n\n' + keydata + b'\n'
> +        keyexport = header + b'\n\n# Trust paths\n\n' + key_paths_repr + keydata + b'\n'

Adding a title is a good idea.

>  
>          if not wotmate.lint(keydata):
>              logger.debug('Skipping %s due to bad linter results', kid)

Best regards
Uwe

Konstantin Ryabitsev March 27, 2025, 8:22 p.m. UTC | #2

On Wed, Mar 26, 2025 at 07:04:06PM +0100, Uwe Kleine-König wrote:
> > -        key_paths_repr = '\n'.join('\n `-> '.join(wotmate.get_uiddata_by_pubrow(c, rowid) if rowid != to_rowid else "." for rowid in kp) for kp in key_paths).encode('utf-8')
> > +        kpblock = ''
> > +        for kp in key_paths:
> > +            for lvl, rowid in enumerate(kp):
> > +                kpuid = wotmate.get_uiddata_by_pubrow(c, rowid)
> > +                kpblock += f'   {lvl}  {kpuid}' if lvl > 0 else f'from  {kpuid}'
> > +                kpblock += '\n'
> > +            kpblock += '\n'
> 
> Now this isn't a one-liner any more you can also work with indention as
> you proposed initially. Something like
> 
>                 kpblock += f'{4 * lvl * " "}`-> {kpuid}' if lvl > 0 else f'{kpuid}'
> 
> (untested).

I tried it and hated it. :)

> > +        key_paths_repr = kpblock.encode()
> 
> Did you drop 'utf-8' here on purpose?

Yes -- it's the default encoding when not specified (and it isn't affected by
the user locale setting). I can put that in explicitly if that makes it more
consistent or expected.

> >          # Do we already have a file in place?
> >          if os.path.exists(keyout):
> >              # Load it up and see if it's different
> >              with open(keyout, 'rb') as fin:
> >                  old_keyexport = fin.read()
> > -                if old_keyexport.find(keydata) > 0 and old_keyexport.find(key_paths_repr) > 0:
> > -                    logger.debug('No changes for %s', kid)
> > -                    continue
> > +                if keydata in old_keyexport:
> > +                    logger.debug('No key changes for %s', kid)
> > +                    if b'\n# Trust paths\n' not in old_keyexport:
> > +                        logger.debug('Old key does not include trust paths, skipping')
> > +                        continue
> > +                    if key_paths_repr in old_keyexport:
> > +                        logger.debug('No trust path changes for %s', kid)
> > +                        continue
> 
> I would add the trust path info already now in a commit changing all
> keys at once. Then we can benefit from it on the next real change of the
> data.

I don't want to do that for a number of reasons:

- this would trigger all keys to be reimported for anyone using
  the korg-refresh-keys
- this would make it harder to quickly judge when the key was last changed

One other possibility is to keep it out of the .asc file and to append this
data to the graph/ file instead, like so:

    ...
    </g>
    </svg>
    <!--
    Trust paths:

    from  Linus Torvalds <[e-mail]>
       1  Greg Kroah-Hartman <[e-mail]>
       2  Kevin Hilman <[e-mail]>
       3  Mattijs Korpershoek <[e-mail]>

    from  Linus Torvalds <[e-mail]>
       1  John Hawley ("Warthog9") <[e-mail]>
       2  Ben Hutchings <[e-mail]>
       3  Uwe Kleine-König <[e-mail]>
       4  Mattijs Korpershoek <[e-mail]>

    -->

What do you think? I like this better, actually, because this will still show
the diffs as we want, but not touch the .asc files (which would trigger their
reimport on trust path changes not necessarily affecting that key).

What do you think?

-K

Uwe Kleine-König March 28, 2025, 10:50 a.m. UTC | #3

On Thu, Mar 27, 2025 at 04:22:03PM -0400, Konstantin Ryabitsev wrote:
> On Wed, Mar 26, 2025 at 07:04:06PM +0100, Uwe Kleine-König wrote:
> > > -        key_paths_repr = '\n'.join('\n `-> '.join(wotmate.get_uiddata_by_pubrow(c, rowid) if rowid != to_rowid else "." for rowid in kp) for kp in key_paths).encode('utf-8')
> > > +        kpblock = ''
> > > +        for kp in key_paths:
> > > +            for lvl, rowid in enumerate(kp):
> > > +                kpuid = wotmate.get_uiddata_by_pubrow(c, rowid)
> > > +                kpblock += f'   {lvl}  {kpuid}' if lvl > 0 else f'from  {kpuid}'
> > > +                kpblock += '\n'
> > > +            kpblock += '\n'
> > 
> > Now this isn't a one-liner any more you can also work with indention as
> > you proposed initially. Something like
> > 
> >                 kpblock += f'{4 * lvl * " "}`-> {kpuid}' if lvl > 0 else f'{kpuid}'
> > 
> > (untested).
> 
> I tried it and hated it. :)

As I didn't test it, I didn't see the result. I expected it to somewhat
match what you suggested earlier. But I don't feel strong here.

> > > +        key_paths_repr = kpblock.encode()
> > 
> > Did you drop 'utf-8' here on purpose?
> 
> Yes -- it's the default encoding when not specified (and it isn't affected by
> the user locale setting). I can put that in explicitly if that makes it more
> consistent or expected.

Ah, I expected the default to be the user's locale. I confirm it's utf-8
and so dropping it is fine.

Related to that, I wonder about:

 a) keys/E2DCDD9132669BD6.asc (and a few others) are latin1 encoded.
    I didn't debug that, but I wonder why these are not utf-8.
 b) The key 4A55C497F744F705 has a uid that is latin1 encoded and 4 that
    are utf-8. See
    `gpg --list-keys --with-colon 4A55C497F744F705 | grep -a ^uid` 

> > >          # Do we already have a file in place?
> > >          if os.path.exists(keyout):
> > >              # Load it up and see if it's different
> > >              with open(keyout, 'rb') as fin:
> > >                  old_keyexport = fin.read()
> > > -                if old_keyexport.find(keydata) > 0 and old_keyexport.find(key_paths_repr) > 0:
> > > -                    logger.debug('No changes for %s', kid)
> > > -                    continue
> > > +                if keydata in old_keyexport:
> > > +                    logger.debug('No key changes for %s', kid)
> > > +                    if b'\n# Trust paths\n' not in old_keyexport:
> > > +                        logger.debug('Old key does not include trust paths, skipping')
> > > +                        continue
> > > +                    if key_paths_repr in old_keyexport:
> > > +                        logger.debug('No trust path changes for %s', kid)
> > > +                        continue
> > 
> > I would add the trust path info already now in a commit changing all
> > keys at once. Then we can benefit from it on the next real change of the
> > data.
> 
> I don't want to do that for a number of reasons:
> 
> - this would trigger all keys to be reimported for anyone using
>   the korg-refresh-keys
> - this would make it harder to quickly judge when the key was last changed
> 
> One other possibility is to keep it out of the .asc file and to append this
> data to the graph/ file instead, like so:
> 
>     ...
>     </g>
>     </svg>
>     <!--
>     Trust paths:
> 
>     from  Linus Torvalds <[e-mail]>
>        1  Greg Kroah-Hartman <[e-mail]>
>        2  Kevin Hilman <[e-mail]>
>        3  Mattijs Korpershoek <[e-mail]>
> 
>     from  Linus Torvalds <[e-mail]>
>        1  John Hawley ("Warthog9") <[e-mail]>
>        2  Ben Hutchings <[e-mail]>
>        3  Uwe Kleine-König <[e-mail]>
>        4  Mattijs Korpershoek <[e-mail]>
> 
>     -->
> 
> What do you think? I like this better, actually, because this will still show
> the diffs as we want, but not touch the .asc files (which would trigger their
> reimport on trust path changes not necessarily affecting that key).

Good idea. This also fixes the problem that grepping for "Linus" in
keys/* isn't a good method to find Linus' key any more with the original
approach.

That brings however the difficulty to detect when it's time to
regenerate the graph. The check could be adapted, but maybe the cleanest
approach is to put the trust path in a completely separate file?

Maybe also put the keyinfo into that new file, given that this doesn't
always represent the current truth when it's only updated on key
changes?

So something like:

	$ cat keys/570338B018144F28.txt
	pub   rsa4096 2022-09-23 [SC] [expires: 2027-03-20]
	      8234A35B45C0D26B31C1A2DA570338B018144F28
	uid                      Mattijs Korpershoek <[e-mail]>
	uid                      Mattijs Korpershoek <[e-mail]>
	uid                      Mattijs Korpershoek <[e-mail]>
	sub   rsa2048 2025-03-20 [S] [expires: 2027-03-20]
	      2EE942A7B61F6ABE5313AD00190D1DB4664E1935
	sub   rsa2048 2025-03-20 [E] [expires: 2027-03-20]
	      F12C50B7B942A1FC14C1AA298A31723BB5DF5B73

	Trust paths:

	from  Linus Torvalds <[e-mail]>
	   1  Greg Kroah-Hartman <[e-mail]>
	   2  Kevin Hilman <[e-mail]>
	   3  Mattijs Korpershoek <[e-mail]>

	from  Linus Torvalds <[e-mail]>
	   1  John Hawley ("Warthog9") <[e-mail]>
	   2  Ben Hutchings <[e-mail]>
	   3  Uwe Kleine-König <[e-mail]>
	   4  Mattijs Korpershoek <[e-mail]>

maybe?

Best regards
Uwe

Patch

diff --git a/export-keyring.py b/export-keyring.py
index 3e455f9..967e1ef 100755
--- a/export-keyring.py
+++ b/export-keyring.py
@@ -110,22 +110,34 @@  if __name__ == '__main__':
             logger.debug('Skipping %s due to invalid WoT', kid)
             continue
 
-        key_paths_repr = '\n'.join('\n `-> '.join(wotmate.get_uiddata_by_pubrow(c, rowid) if rowid != to_rowid else "." for rowid in kp) for kp in key_paths).encode('utf-8')
+        kpblock = ''
+        for kp in key_paths:
+            for lvl, rowid in enumerate(kp):
+                kpuid = wotmate.get_uiddata_by_pubrow(c, rowid)
+                kpblock += f'   {lvl}  {kpuid}' if lvl > 0 else f'from  {kpuid}'
+                kpblock += '\n'
+            kpblock += '\n'
+        key_paths_repr = kpblock.encode()
 
         # Do we already have a file in place?
         if os.path.exists(keyout):
             # Load it up and see if it's different
             with open(keyout, 'rb') as fin:
                 old_keyexport = fin.read()
-                if old_keyexport.find(keydata) > 0 and old_keyexport.find(key_paths_repr) > 0:
-                    logger.debug('No changes for %s', kid)
-                    continue
+                if keydata in old_keyexport:
+                    logger.debug('No key changes for %s', kid)
+                    if b'\n# Trust paths\n' not in old_keyexport:
+                        logger.debug('Old key does not include trust paths, skipping')
+                        continue
+                    if key_paths_repr in old_keyexport:
+                        logger.debug('No trust path changes for %s', kid)
+                        continue
 
         # Now, export the header
         args = ['--list-options', 'show-notations', '--list-options',
                 'no-show-uid-validity', '--with-subkey-fingerprints', '--list-key', kid]
         header = wotmate.gpg_run_command(args, with_colons=False)
-        keyexport = header + b'\n\n' + key_paths_repr + b'\n\n' + keydata + b'\n'
+        keyexport = header + b'\n\n# Trust paths\n\n' + key_paths_repr + keydata + b'\n'
 
         if not wotmate.lint(keydata):
             logger.debug('Skipping %s due to bad linter results', kid)