NewTypes for clearer encoding types #474

emilejbm · 2023-01-26T20:12:43Z

Issue #249

Summary

Using newtype for certain parameters that were previously only str will make the API more misuse-resistant

Release Note

NONE

Documentation

NewTypes can be documented so that users can verify that return types are of expected types

…to main

…to main added newtypes to _util

…to main added newtypes to verify/models.py

woodruffw · 2023-01-26T21:06:56Z

find_keywords.sh

+#/bin/bash
+
+grep -nr "b64\|base64\|cert_pem\|pem" sigstore/* test/* > key_lines.txt


Nit: fine for now, but this should be dropped from the changeset once it's ready for a final review 🙂

woodruffw · 2023-01-26T21:07:32Z

sigstore/_utils.py

@@ -32,6 +32,8 @@
 else:
    from importlib import resources

+import newtype


Nonexistent module? I see you added newtypes.py so this should probably be import newtypes (but I'll leave a comment w/ thoughts about avoiding a new module).

woodruffw · 2023-01-26T21:09:37Z

sigstore/newtypes.py

+Hexstr = NewType('hexstr', str)
+B64str = NewType('b64str', str)
+Pemcert = NewType("pemcert", str)
+KeyID = NewType('keyID', bytes)


The NewType API expects the LHS of the expression and the first argument to match, so these should be:

Suggested change

Hexstr = NewType('hexstr', str)

B64str = NewType('b64str', str)

Pemcert = NewType("pemcert", str)

KeyID = NewType('keyID', bytes)

hexstr = NewType('hexstr', str)

b64str = NewType('b64str', str)

pemcert = NewType("pemcert", str)

keyid = NewType('keyid', bytes)

(I made these all lowercase because they're all NewTypes of primitive types, but it might make sense to have them be PEMCert etc. instead. But we can think more about that in a later review.)

woodruffw · 2023-01-26T21:10:30Z

sigstore/newtypes.py

@@ -0,0 +1,6 @@
+from typing import NewType


My preference would be to avoid a new module here (especially a public one) -- IMO these newtypes should go in the existing _utils module, since that's where a lot of them get constructed anyways.

Should we then import _utils instead of import newtypes in every file that needs changes? I am wondering if there is a way of putting all the needed newtypes in a function and doing from _utils import function instead.

Should we then import _utils instead of import newtypes in every file that needs changes?

Yes, exactly (although you should use the fully qualified sigstore._utils path for consistency with the rest of the code).

I am wondering if there is a way of putting all the needed newtypes in a function and doing from _utils import function instead.

This is probably overkill -- having them at the top-level of _utils is idiomatic, and we can selectively import them as needed.

woodruffw · 2023-01-26T21:12:17Z

Thanks for opening this! Looks like a good first start; I left a couple of design nits and suggested changes.

FYI: there are a couple of things that our QA tools would have caught -- if you run make lint locally, you should be able to follow the output there to get things fixed.

(You can also run make reformat to automatically run code formatting.)

woodruffw · 2023-01-31T19:01:42Z

sigstore/sign.py

@@ -73,6 +73,7 @@
 from sigstore._internal.tuf import TrustUpdater
 from sigstore._utils import sha256_streaming
 from sigstore.transparency import LogEntry
+import newtypes 


Nit: remove entirely and replace with imports from sigstore._utils.

woodruffw · 2023-01-31T19:01:55Z

sigstore/verify/models.py

@@ -31,6 +31,8 @@
 from sigstore._utils import base64_encode_pem_cert, sha256_streaming
 from sigstore.transparency import LogEntry

+from sigstore import newtypes


Nit: same as https://github.com/sigstore/sigstore-python/pull/474/files#r1092359520

…fier

woodruffw · 2023-02-01T04:53:59Z

sigstore/sign.py

+from sigstore._utils import hexstr
+from sigstore._utils import b64str
+from sigstore._utils import pemcert
+from sigstore._utils import keyid


These should be imported in a single line; make reformat should do this for you automatically.

…igstore#466) Updates the requirements on [ruff](https://github.com/charliermarsh/ruff) to permit the latest version. - [Release notes](https://github.com/charliermarsh/ruff/releases) - [Changelog](https://github.com/charliermarsh/ruff/blob/main/BREAKING_CHANGES.md) - [Commits](astral-sh/ruff@v0.0.18...v0.0.228) --- updated-dependencies: - dependency-name: ruff dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: emboman13 <embo1013@yahoo.com>

…igstore#468) Updates the requirements on [ruff](https://github.com/charliermarsh/ruff) to permit the latest version. - [Release notes](https://github.com/charliermarsh/ruff/releases) - [Changelog](https://github.com/charliermarsh/ruff/blob/main/BREAKING_CHANGES.md) - [Commits](astral-sh/ruff@v0.0.18...v0.0.230) --- updated-dependencies: - dependency-name: ruff dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: emboman13 <embo1013@yahoo.com>

…igstore#469) Updates the requirements on [ruff](https://github.com/charliermarsh/ruff) to permit the latest version. - [Release notes](https://github.com/charliermarsh/ruff/releases) - [Changelog](https://github.com/charliermarsh/ruff/blob/main/BREAKING_CHANGES.md) - [Commits](astral-sh/ruff@v0.0.18...v0.0.231) --- updated-dependencies: - dependency-name: ruff dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: emboman13 <embo1013@yahoo.com>

* Initial Sigstore bundle support Signed-off-by: William Woodruff <william@trailofbits.com> * README: update `--help` texts Signed-off-by: William Woodruff <william@trailofbits.com> * sign: fix bundle generation Certs are base64'd DER, not PEM, and the canonicalized_body is the log entry body, not the canonicalized contents that the SET is signed over. Signed-off-by: William Woodruff <william@trailofbits.com> * sign: remove TODO Signed-off-by: William Woodruff <william@trailofbits.com> * sign: update TODO Signed-off-by: William Woodruff <william@trailofbits.com> * _cli: Make `--bundle` refer to a path and create a `--no-bundle` flag to control whether Sigstore bundles are emitted by default Signed-off-by: Alex Cameron <asc@tetsuo.sh> * _cli: Move variable to correct scope Signed-off-by: Alex Cameron <asc@tetsuo.sh> * _cli: Reword warnings for bundle flags Signed-off-by: Alex Cameron <asc@tetsuo.sh> * README: Fix sign example Signed-off-by: Alex Cameron <asc@tetsuo.sh> * README: Update verify invocations Signed-off-by: Alex Cameron <asc@tetsuo.sh> * README: Fix line breaks Signed-off-by: Alex Cameron <asc@tetsuo.sh> * _cli: fix sig output Signed-off-by: William Woodruff <william@trailofbits.com> * _cli: fix sig check, take 2 Signed-off-by: William Woodruff <william@trailofbits.com> Signed-off-by: William Woodruff <william@trailofbits.com> Signed-off-by: Alex Cameron <asc@tetsuo.sh> Co-authored-by: Alex Cameron <asc@tetsuo.sh> Signed-off-by: emboman13 <embo1013@yahoo.com>

Signed-off-by: omartounsi7 <otounsi@purdue.edu>

omartounsi7 · 2023-02-08T20:40:48Z

@woodruffw Hello William, we have pushed all of our changes. But there is a conflict with _internal/rekor/client.py which we don't know how to resolve. There is a class RekorBundle that exists on our branch but not on the main branch. Other than that, we think we are ready for a code review.

woodruffw · 2023-02-08T21:40:30Z

There is a class RekorBundle that exists on our branch but not on the main branch.

You can remove that class and anything related to it from your branch -- that's part of an old internal API that we've removed.

woodruffw · 2023-02-08T22:13:56Z

sigstore/_internal/ctfe.py

@@ -25,7 +25,7 @@
 from cryptography.hazmat.primitives import hashes
 from cryptography.hazmat.primitives.asymmetric import ec, rsa

-from sigstore._utils import key_id, load_pem_public_key
+from sigstore._utils import key_id, keyid, load_pem_public_key


Hmm, having both key_id (a function) and keyid (a type) is definitely a little confusing.

Let's go with CamelCase for these types after all:

keyid -> KeyID

hexstr -> HexStr

b64str -> B64Str

pemcert -> PEMCert

dercert -> DERCert

woodruffw · 2023-02-08T22:15:29Z

sigstore/_utils.py

 PublicKey = Union[rsa.RSAPublicKey, ec.EllipticCurvePublicKey]

+hexstr = NewType("hexstr", str)


These won't be public APIs, but let's add docstrings for them anyways, e.g.:

Suggested change

hexstr = NewType("hexstr", str)

hexstr = NewType("hexstr", str)

"""

A newtype for `str` objects that contain hexadecimal strings (e.g. `ffabcd00ff`).

"""

Signed-off-by: omartounsi7 <62721212+omartounsi7@users.noreply.github.com>

Signed-off-by: omartounsi7 <otounsi@purdue.edu>

omartounsi7 · 2023-02-09T07:18:02Z

We've implemented the requested changes. Are the docstrings good?

sigstore/_utils.py

woodruffw · 2023-02-10T23:22:29Z

We've implemented the requested changes. Are the docstrings good?

Two small errors that I've made suggestions for, otherwise looks good! Once you get those fixed feel free to remove the draft status and we'll do a final review.

Co-authored-by: William Woodruff <william@yossarian.net> Signed-off-by: omartounsi7 <62721212+omartounsi7@users.noreply.github.com>

woodruffw · 2023-02-14T17:22:00Z

/gcbrun

woodruffw · 2023-02-14T17:24:45Z

Thanks all! Great work.

emilejbm and others added 14 commits January 26, 2023 09:42

class prototype and script to find instances to change

bd24fb3

script added

2c99e6b

Set up newtype file with example newTypes

d677a3d

added newtypes to _util.py

cdef65f

renamed newtypes, added keyID

4ca71e5

Merge branch 'main' of https://github.com/emilejbm/sigstore-python in…

e9d809c

…to main

deletion of old file

2ab8f61

Merge branch 'main' of https://github.com/emilejbm/sigstore-python in…

e83f1d3

…to main added newtypes to _util

added hexstr newtype and implemented newtypes for SigningResults

8a9f643

Merge branch 'main' of https://github.com/emilejbm/sigstore-python

b34d64b

added newtypes to verify/models.py

cafb2e0

Merge branch 'main' of https://github.com/emilejbm/sigstore-python in…

8e22f40

…to main added newtypes to verify/models.py

renamed newtypes to follow standardized format

d035147

Merge branch 'main' of https://github.com/emilejbm/sigstore-python

ca77e83

woodruffw reviewed Jan 26, 2023

View reviewed changes

woodruffw added the refactoring Refactoring tasks. label Jan 26, 2023

moved newtypes into _util

429c836

woodruffw reviewed Jan 31, 2023

View reviewed changes

omartounsi7 and others added 2 commits January 31, 2023 14:39

deleted newtypes.py

6c30e5b

Changed sign.py to use _utils and set up basic implementation in veri…

6b180eb

…fier

woodruffw reviewed Feb 1, 2023

View reviewed changes

dependabot bot and others added 4 commits February 2, 2023 09:48

omartounsi7 added 3 commits February 8, 2023 15:01

added a b64str newtype in _internal/oidc/oauth.py

78a88a1

Signed-off-by: omartounsi7 <otounsi@purdue.edu>

added a b64str newtype in _internal/rekor/client.py

02b96b1

Signed-off-by: omartounsi7 <otounsi@purdue.edu>

deleted script

6437bea

Signed-off-by: omartounsi7 <otounsi@purdue.edu>

woodruffw reviewed Feb 8, 2023

View reviewed changes

omartounsi7 and others added 8 commits February 8, 2023 18:25

Merge branch 'main' into main

fb1c97d

Signed-off-by: omartounsi7 <62721212+omartounsi7@users.noreply.github.com>

fixed some type errors

67bf74e

changed keyid to KeyID

3ecabbe

Signed-off-by: omartounsi7 <otounsi@purdue.edu>

anged hexstr to HexStr

a5afe85

Signed-off-by: omartounsi7 <otounsi@purdue.edu>

changed b64str to B64Str

aa7c6a9

Signed-off-by: omartounsi7 <otounsi@purdue.edu>

changed pemcert to PEMCert

67481d2

Signed-off-by: omartounsi7 <otounsi@purdue.edu>

changed dercert to DERCert

92127e5

Signed-off-by: omartounsi7 <otounsi@purdue.edu>

added docstrings to newtypes in _utils.py

d53b410

Signed-off-by: omartounsi7 <otounsi@purdue.edu>

woodruffw reviewed Feb 10, 2023

View reviewed changes

sigstore/_utils.py Outdated Show resolved Hide resolved

woodruffw reviewed Feb 10, 2023

View reviewed changes

sigstore/_utils.py Outdated Show resolved Hide resolved

omartounsi7 and others added 2 commits February 10, 2023 22:17

Update sigstore/_utils.py

7e66170

Co-authored-by: William Woodruff <william@yossarian.net> Signed-off-by: omartounsi7 <62721212+omartounsi7@users.noreply.github.com>

Update sigstore/_utils.py

f46e418

Co-authored-by: William Woodruff <william@yossarian.net> Signed-off-by: omartounsi7 <62721212+omartounsi7@users.noreply.github.com>

emilejbm marked this pull request as ready for review February 11, 2023 18:21

emilejbm changed the title ~~Draft PR for confirmation on NewType usage~~ NewTypes for clearer encoding types Feb 11, 2023

Merge branch 'main' into main

49f5502

woodruffw added the safe to test label Feb 14, 2023

woodruffw approved these changes Feb 14, 2023

View reviewed changes

woodruffw merged commit 71ca534 into sigstore:main Feb 14, 2023

woodruffw mentioned this pull request Feb 14, 2023

Chore: use newtypes to make the API more misuse-resistant #249

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NewTypes for clearer encoding types #474

NewTypes for clearer encoding types #474

emilejbm commented Jan 26, 2023

woodruffw Jan 26, 2023

woodruffw Jan 26, 2023

woodruffw Jan 26, 2023

woodruffw Jan 26, 2023

omartounsi7 Jan 31, 2023

woodruffw Jan 31, 2023

woodruffw commented Jan 26, 2023

woodruffw Jan 31, 2023

woodruffw Jan 31, 2023

woodruffw Feb 1, 2023

omartounsi7 commented Feb 8, 2023

woodruffw commented Feb 8, 2023

woodruffw Feb 8, 2023

woodruffw Feb 8, 2023

omartounsi7 commented Feb 9, 2023

woodruffw commented Feb 10, 2023

woodruffw commented Feb 14, 2023

woodruffw commented Feb 14, 2023

		#/bin/bash

		grep -nr "b64\\|base64\\|cert_pem\\|pem" sigstore/* test/* > key_lines.txt

		PublicKey = Union[rsa.RSAPublicKey, ec.EllipticCurvePublicKey]

		hexstr = NewType("hexstr", str)

NewTypes for clearer encoding types #474

NewTypes for clearer encoding types #474

Conversation

emilejbm commented Jan 26, 2023

Summary

Release Note

Documentation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woodruffw commented Jan 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omartounsi7 commented Feb 8, 2023

woodruffw commented Feb 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omartounsi7 commented Feb 9, 2023

woodruffw commented Feb 10, 2023

woodruffw commented Feb 14, 2023

woodruffw commented Feb 14, 2023