chore(deps): bump signature to 3.0.0-pre - #505

Closed
baloo wants to merge 1 commit into
RustCrypto:masterfrom
baloo:baloo/signature/3
Closed

chore(deps): bump signature to 3.0.0-pre#505
baloo wants to merge 1 commit into
RustCrypto:masterfrom
baloo:baloo/signature/3

Conversation

@baloo

Copy link
Copy Markdown
Member

No description provided.

@baloo
baloo marked this pull request as draft April 23, 2025 21:46
@baloo
balooforce-pushed the baloo/signature/3 branch 2 times, most recently from eaaea51 to e81c85aCompareApril 23, 2025 22:31
@baloo
baloo marked this pull request as ready for review April 23, 2025 22:32
@baloo
balooforce-pushed the baloo/signature/3 branch from e81c85a to 22a4bd9CompareApril 23, 2025 22:34
@tarcieri

Copy link
Copy Markdown
Member

Ugh, that’s really annoying if the blanket impl leads to an inference failure.

Regarding carrying the digest, perhaps something like https://docs.rs/ecdsa/latest/ecdsa/struct.SignatureWithOid.html ?

@tarcieri

Copy link
Copy Markdown
Member

I opened RustCrypto/traits#1831 to discuss the inference regression. We might want to consider backing out this change.

@baloo

baloo commented Apr 24, 2025

Copy link
Copy Markdown
MemberAuthor

I'm not sure I get what you're trying to suggest with SignatureWithOid.

I think this overall is a side effect of having to provide the Digest to the blanket Signer, which you can't provide unless you specify that through the resulting type. (Because there could be more than one DigestSigner implemented).

I otherwise think that specifying the digest attached to a signature makes sense (and that we should actually embrace that change, whether or not we fix the inference for it).

@tarcieri

Copy link
Copy Markdown
Member

One thing that would be weird about attaching a digest type to the signature type is algorithm information isn't available in the serialized signature, so you'll be able to parse signatures generated by other digest algorithms successfully

@baloo

baloo commented Apr 24, 2025

Copy link
Copy Markdown
MemberAuthor

Yeah but that should be inferred from the VerifierDigest call that should ensue (otherwise the digest can be whatever)

Note: Neither the ecdsa::signature nor ecdsa::der::Signature have it either they don’t have digest in the first place

@baloo

Copy link
Copy Markdown
MemberAuthor

Wait, we do have the digest in the SigningKey, there should not be an issue with selecting the DigestSigner. I’ll have another look tomorrow.

@baloo

Copy link
Copy Markdown
MemberAuthor

#505 (comment)
That was actually a red herring, I didn't have PrehashSignature for Signature in the first place, so I could not use the blanket. The only way to implement PrehashSignature would here be to have signature carry the associated digest (which I did).
The type inference works just fine after that.

We get away with not having the Digest in ecdsa::Signature because the associated Digest is used instead.

#[derive(Debug, Clone, PartialEq, Eq)]
pub struct Signature {
#[derive(Eq)]
pub struct Signature<D> {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This still seems a little weird to me because there isn't a strong binding or relationship between the signature as a cryptographic object and the digest algorithm that was used to compute it.

This means that the type does not actually maintain an invariant e.g. "this is a signature that was known to be computed by using digest D over the input message". It could've been computed with any digest algorithm.

I guess there's a type safety argument to it in that the type identifies what digest you're supposed to use, but as cryptographic objects they're not really parameterized/distinguished by the digest, it's just something that happens earlier in the computation of the signature.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@baloo WDYT?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I don’t know what I think ^^.
I know that I don’t really have much of a choice.

What I’m arguing it that it makes the digest explicit when accepting/parsing a signature: “I know this is going to be verified with this public key with sha384”

I know the serialization does not necessarily carry this information, although it would be carried via the OID on the object (x509 or cms) or via a specification or however the developer may which. Whichever that might be the digest will need to be provided for the VerifyingKey.

all in all this is a little bit inconvenient, but not all that much and it just makes the digest choice explicit.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess the only reason it exists is for the PrehashSignature impl.

I find it a little troubling it otherwise doesn't actually do anything, but I guess we need to merge this to make any progress.

Comment on lines -146 to -154
#[cfg(feature = "os_rng")]
impl<D> Signer<Signature> for SigningKey<D>
where
D: Digest + FixedOutputReset,
{
fn try_sign(&self, msg: &[u8]) -> signature::Result<Signature> {
self.try_sign_with_rng(&mut OsRng, msg)
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess the alternative would be to preserve these Signer impls and not impl PrehashSignature for Signature since it's not actually known at the type level

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Iirc I tried that, but I came in conflict with the blanket implementation.

can’t try it right this moment, will try that in an hour or so.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you don't impl PrehashSignature the blanket impl shouldn't matter

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

yeah looks like it works, but that still mess up the type inference and you need to provide the type of the signature you expect. You don't need to provide the digest thought.

#510

@baloobaloo mentioned this pull request Apr 30, 2025
@baloo

baloo commented May 2, 2025

Copy link
Copy Markdown
MemberAuthor

closed in favor of #513

@baloobaloo closed this May 2, 2025
@baloo
baloo deleted the baloo/signature/3 branch May 2, 2025 22:43
Sign up for freeto 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.

2 participants

@baloo@tarcieri
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

chore(deps): bump signature to 3.0.0-pre - #505

Closed
baloo wants to merge 1 commit into
RustCrypto:masterfrom
baloo:baloo/signature/3
Closed

chore(deps): bump signature to 3.0.0-pre#505
baloo wants to merge 1 commit into
RustCrypto:masterfrom
baloo:baloo/signature/3

Conversation

@baloo

Copy link
Copy Markdown
Member

No description provided.

@baloo
baloo marked this pull request as draft April 23, 2025 21:46
@baloo
balooforce-pushed the baloo/signature/3 branch 2 times, most recently from eaaea51 to e81c85aCompareApril 23, 2025 22:31
@baloo
baloo marked this pull request as ready for review April 23, 2025 22:32
@baloo
balooforce-pushed the baloo/signature/3 branch from e81c85a to 22a4bd9CompareApril 23, 2025 22:34
@tarcieri

Copy link
Copy Markdown
Member

Ugh, that’s really annoying if the blanket impl leads to an inference failure.

Regarding carrying the digest, perhaps something like https://docs.rs/ecdsa/latest/ecdsa/struct.SignatureWithOid.html ?

@tarcieri

Copy link
Copy Markdown
Member

I opened RustCrypto/traits#1831 to discuss the inference regression. We might want to consider backing out this change.

@baloo

baloo commented Apr 24, 2025

Copy link
Copy Markdown
MemberAuthor

I'm not sure I get what you're trying to suggest with SignatureWithOid.

I think this overall is a side effect of having to provide the Digest to the blanket Signer, which you can't provide unless you specify that through the resulting type. (Because there could be more than one DigestSigner implemented).

I otherwise think that specifying the digest attached to a signature makes sense (and that we should actually embrace that change, whether or not we fix the inference for it).

@tarcieri

Copy link
Copy Markdown
Member

One thing that would be weird about attaching a digest type to the signature type is algorithm information isn't available in the serialized signature, so you'll be able to parse signatures generated by other digest algorithms successfully

@baloo

baloo commented Apr 24, 2025

Copy link
Copy Markdown
MemberAuthor

Yeah but that should be inferred from the VerifierDigest call that should ensue (otherwise the digest can be whatever)

Note: Neither the ecdsa::signature nor ecdsa::der::Signature have it either they don’t have digest in the first place

@baloo

Copy link
Copy Markdown
MemberAuthor

Wait, we do have the digest in the SigningKey, there should not be an issue with selecting the DigestSigner. I’ll have another look tomorrow.

@baloo

Copy link
Copy Markdown
MemberAuthor

#505 (comment)
That was actually a red herring, I didn't have PrehashSignature for Signature in the first place, so I could not use the blanket. The only way to implement PrehashSignature would here be to have signature carry the associated digest (which I did).
The type inference works just fine after that.

We get away with not having the Digest in ecdsa::Signature because the associated Digest is used instead.

#[derive(Debug, Clone, PartialEq, Eq)]
pub struct Signature {
#[derive(Eq)]
pub struct Signature<D> {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This still seems a little weird to me because there isn't a strong binding or relationship between the signature as a cryptographic object and the digest algorithm that was used to compute it.

This means that the type does not actually maintain an invariant e.g. "this is a signature that was known to be computed by using digest D over the input message". It could've been computed with any digest algorithm.

I guess there's a type safety argument to it in that the type identifies what digest you're supposed to use, but as cryptographic objects they're not really parameterized/distinguished by the digest, it's just something that happens earlier in the computation of the signature.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@baloo WDYT?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I don’t know what I think ^^.
I know that I don’t really have much of a choice.

What I’m arguing it that it makes the digest explicit when accepting/parsing a signature: “I know this is going to be verified with this public key with sha384”

I know the serialization does not necessarily carry this information, although it would be carried via the OID on the object (x509 or cms) or via a specification or however the developer may which. Whichever that might be the digest will need to be provided for the VerifyingKey.

all in all this is a little bit inconvenient, but not all that much and it just makes the digest choice explicit.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess the only reason it exists is for the PrehashSignature impl.

I find it a little troubling it otherwise doesn't actually do anything, but I guess we need to merge this to make any progress.

Comment on lines -146 to -154
#[cfg(feature = "os_rng")]
impl<D> Signer<Signature> for SigningKey<D>
where
D: Digest + FixedOutputReset,
{
fn try_sign(&self, msg: &[u8]) -> signature::Result<Signature> {
self.try_sign_with_rng(&mut OsRng, msg)
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess the alternative would be to preserve these Signer impls and not impl PrehashSignature for Signature since it's not actually known at the type level

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Iirc I tried that, but I came in conflict with the blanket implementation.

can’t try it right this moment, will try that in an hour or so.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you don't impl PrehashSignature the blanket impl shouldn't matter

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

yeah looks like it works, but that still mess up the type inference and you need to provide the type of the signature you expect. You don't need to provide the digest thought.

#510

@baloobaloo mentioned this pull request Apr 30, 2025
@baloo

baloo commented May 2, 2025

Copy link
Copy Markdown
MemberAuthor

closed in favor of #513

@baloobaloo closed this May 2, 2025
@baloo
baloo deleted the baloo/signature/3 branch May 2, 2025 22:43
Sign up for freeto 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.

2 participants

@baloo@tarcieri
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

chore(deps): bump signature to 3.0.0-pre - #505

Closed
baloo wants to merge 1 commit into
RustCrypto:masterfrom
baloo:baloo/signature/3
Closed

chore(deps): bump signature to 3.0.0-pre#505
baloo wants to merge 1 commit into
RustCrypto:masterfrom
baloo:baloo/signature/3

Conversation

@baloo

Copy link
Copy Markdown
Member

No description provided.

@baloo
baloo marked this pull request as draft April 23, 2025 21:46
@baloo
balooforce-pushed the baloo/signature/3 branch 2 times, most recently from eaaea51 to e81c85aCompareApril 23, 2025 22:31
@baloo
baloo marked this pull request as ready for review April 23, 2025 22:32
@baloo
balooforce-pushed the baloo/signature/3 branch from e81c85a to 22a4bd9CompareApril 23, 2025 22:34
@tarcieri

Copy link
Copy Markdown
Member

Ugh, that’s really annoying if the blanket impl leads to an inference failure.

Regarding carrying the digest, perhaps something like https://docs.rs/ecdsa/latest/ecdsa/struct.SignatureWithOid.html ?

@tarcieri

Copy link
Copy Markdown
Member

I opened RustCrypto/traits#1831 to discuss the inference regression. We might want to consider backing out this change.

@baloo

baloo commented Apr 24, 2025

Copy link
Copy Markdown
MemberAuthor

I'm not sure I get what you're trying to suggest with SignatureWithOid.

I think this overall is a side effect of having to provide the Digest to the blanket Signer, which you can't provide unless you specify that through the resulting type. (Because there could be more than one DigestSigner implemented).

I otherwise think that specifying the digest attached to a signature makes sense (and that we should actually embrace that change, whether or not we fix the inference for it).

@tarcieri

Copy link
Copy Markdown
Member

One thing that would be weird about attaching a digest type to the signature type is algorithm information isn't available in the serialized signature, so you'll be able to parse signatures generated by other digest algorithms successfully

@baloo

baloo commented Apr 24, 2025

Copy link
Copy Markdown
MemberAuthor

Yeah but that should be inferred from the VerifierDigest call that should ensue (otherwise the digest can be whatever)

Note: Neither the ecdsa::signature nor ecdsa::der::Signature have it either they don’t have digest in the first place

@baloo

Copy link
Copy Markdown
MemberAuthor

Wait, we do have the digest in the SigningKey, there should not be an issue with selecting the DigestSigner. I’ll have another look tomorrow.

@baloo

Copy link
Copy Markdown
MemberAuthor

#505 (comment)
That was actually a red herring, I didn't have PrehashSignature for Signature in the first place, so I could not use the blanket. The only way to implement PrehashSignature would here be to have signature carry the associated digest (which I did).
The type inference works just fine after that.

We get away with not having the Digest in ecdsa::Signature because the associated Digest is used instead.

#[derive(Debug, Clone, PartialEq, Eq)]
pub struct Signature {
#[derive(Eq)]
pub struct Signature<D> {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This still seems a little weird to me because there isn't a strong binding or relationship between the signature as a cryptographic object and the digest algorithm that was used to compute it.

This means that the type does not actually maintain an invariant e.g. "this is a signature that was known to be computed by using digest D over the input message". It could've been computed with any digest algorithm.

I guess there's a type safety argument to it in that the type identifies what digest you're supposed to use, but as cryptographic objects they're not really parameterized/distinguished by the digest, it's just something that happens earlier in the computation of the signature.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@baloo WDYT?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I don’t know what I think ^^.
I know that I don’t really have much of a choice.

What I’m arguing it that it makes the digest explicit when accepting/parsing a signature: “I know this is going to be verified with this public key with sha384”

I know the serialization does not necessarily carry this information, although it would be carried via the OID on the object (x509 or cms) or via a specification or however the developer may which. Whichever that might be the digest will need to be provided for the VerifyingKey.

all in all this is a little bit inconvenient, but not all that much and it just makes the digest choice explicit.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess the only reason it exists is for the PrehashSignature impl.

I find it a little troubling it otherwise doesn't actually do anything, but I guess we need to merge this to make any progress.

Comment on lines -146 to -154
#[cfg(feature = "os_rng")]
impl<D> Signer<Signature> for SigningKey<D>
where
D: Digest + FixedOutputReset,
{
fn try_sign(&self, msg: &[u8]) -> signature::Result<Signature> {
self.try_sign_with_rng(&mut OsRng, msg)
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess the alternative would be to preserve these Signer impls and not impl PrehashSignature for Signature since it's not actually known at the type level

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Iirc I tried that, but I came in conflict with the blanket implementation.

can’t try it right this moment, will try that in an hour or so.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you don't impl PrehashSignature the blanket impl shouldn't matter

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

yeah looks like it works, but that still mess up the type inference and you need to provide the type of the signature you expect. You don't need to provide the digest thought.

#510

@baloobaloo mentioned this pull request Apr 30, 2025
@baloo

baloo commented May 2, 2025

Copy link
Copy Markdown
MemberAuthor

closed in favor of #513

@baloobaloo closed this May 2, 2025
@baloo
baloo deleted the baloo/signature/3 branch May 2, 2025 22:43
Sign up for freeto 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.

2 participants

@baloo@tarcieri
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

chore(deps): bump signature to 3.0.0-pre - #505

Closed
baloo wants to merge 1 commit into
RustCrypto:masterfrom
baloo:baloo/signature/3
Closed

chore(deps): bump signature to 3.0.0-pre#505
baloo wants to merge 1 commit into
RustCrypto:masterfrom
baloo:baloo/signature/3

Conversation

@baloo

Copy link
Copy Markdown
Member

No description provided.

@baloo
baloo marked this pull request as draft April 23, 2025 21:46
@baloo
balooforce-pushed the baloo/signature/3 branch 2 times, most recently from eaaea51 to e81c85aCompareApril 23, 2025 22:31
@baloo
baloo marked this pull request as ready for review April 23, 2025 22:32
@baloo
balooforce-pushed the baloo/signature/3 branch from e81c85a to 22a4bd9CompareApril 23, 2025 22:34
@tarcieri

Copy link
Copy Markdown
Member

Ugh, that’s really annoying if the blanket impl leads to an inference failure.

Regarding carrying the digest, perhaps something like https://docs.rs/ecdsa/latest/ecdsa/struct.SignatureWithOid.html ?

@tarcieri

Copy link
Copy Markdown
Member

I opened RustCrypto/traits#1831 to discuss the inference regression. We might want to consider backing out this change.

@baloo

baloo commented Apr 24, 2025

Copy link
Copy Markdown
MemberAuthor

I'm not sure I get what you're trying to suggest with SignatureWithOid.

I think this overall is a side effect of having to provide the Digest to the blanket Signer, which you can't provide unless you specify that through the resulting type. (Because there could be more than one DigestSigner implemented).

I otherwise think that specifying the digest attached to a signature makes sense (and that we should actually embrace that change, whether or not we fix the inference for it).

@tarcieri

Copy link
Copy Markdown
Member

One thing that would be weird about attaching a digest type to the signature type is algorithm information isn't available in the serialized signature, so you'll be able to parse signatures generated by other digest algorithms successfully

@baloo

baloo commented Apr 24, 2025

Copy link
Copy Markdown
MemberAuthor

Yeah but that should be inferred from the VerifierDigest call that should ensue (otherwise the digest can be whatever)

Note: Neither the ecdsa::signature nor ecdsa::der::Signature have it either they don’t have digest in the first place

@baloo

Copy link
Copy Markdown
MemberAuthor

Wait, we do have the digest in the SigningKey, there should not be an issue with selecting the DigestSigner. I’ll have another look tomorrow.

@baloo

Copy link
Copy Markdown
MemberAuthor

#505 (comment)
That was actually a red herring, I didn't have PrehashSignature for Signature in the first place, so I could not use the blanket. The only way to implement PrehashSignature would here be to have signature carry the associated digest (which I did).
The type inference works just fine after that.

We get away with not having the Digest in ecdsa::Signature because the associated Digest is used instead.

#[derive(Debug, Clone, PartialEq, Eq)]
pub struct Signature {
#[derive(Eq)]
pub struct Signature<D> {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This still seems a little weird to me because there isn't a strong binding or relationship between the signature as a cryptographic object and the digest algorithm that was used to compute it.

This means that the type does not actually maintain an invariant e.g. "this is a signature that was known to be computed by using digest D over the input message". It could've been computed with any digest algorithm.

I guess there's a type safety argument to it in that the type identifies what digest you're supposed to use, but as cryptographic objects they're not really parameterized/distinguished by the digest, it's just something that happens earlier in the computation of the signature.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@baloo WDYT?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I don’t know what I think ^^.
I know that I don’t really have much of a choice.

What I’m arguing it that it makes the digest explicit when accepting/parsing a signature: “I know this is going to be verified with this public key with sha384”

I know the serialization does not necessarily carry this information, although it would be carried via the OID on the object (x509 or cms) or via a specification or however the developer may which. Whichever that might be the digest will need to be provided for the VerifyingKey.

all in all this is a little bit inconvenient, but not all that much and it just makes the digest choice explicit.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess the only reason it exists is for the PrehashSignature impl.

I find it a little troubling it otherwise doesn't actually do anything, but I guess we need to merge this to make any progress.

Comment on lines -146 to -154
#[cfg(feature = "os_rng")]
impl<D> Signer<Signature> for SigningKey<D>
where
D: Digest + FixedOutputReset,
{
fn try_sign(&self, msg: &[u8]) -> signature::Result<Signature> {
self.try_sign_with_rng(&mut OsRng, msg)
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess the alternative would be to preserve these Signer impls and not impl PrehashSignature for Signature since it's not actually known at the type level

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Iirc I tried that, but I came in conflict with the blanket implementation.

can’t try it right this moment, will try that in an hour or so.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you don't impl PrehashSignature the blanket impl shouldn't matter

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

yeah looks like it works, but that still mess up the type inference and you need to provide the type of the signature you expect. You don't need to provide the digest thought.

#510

@baloobaloo mentioned this pull request Apr 30, 2025
@baloo

baloo commented May 2, 2025

Copy link
Copy Markdown
MemberAuthor

closed in favor of #513

@baloobaloo closed this May 2, 2025
@baloo
baloo deleted the baloo/signature/3 branch May 2, 2025 22:43
Sign up for freeto 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.

2 participants

@baloo@tarcieri
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

chore(deps): bump signature to 3.0.0-pre - #505

Closed
baloo wants to merge 1 commit into
RustCrypto:masterfrom
baloo:baloo/signature/3
Closed

chore(deps): bump signature to 3.0.0-pre#505
baloo wants to merge 1 commit into
RustCrypto:masterfrom
baloo:baloo/signature/3

Conversation

@baloo

Copy link
Copy Markdown
Member

No description provided.

@baloo
baloo marked this pull request as draft April 23, 2025 21:46
@baloo
balooforce-pushed the baloo/signature/3 branch 2 times, most recently from eaaea51 to e81c85aCompareApril 23, 2025 22:31
@baloo
baloo marked this pull request as ready for review April 23, 2025 22:32
@baloo
balooforce-pushed the baloo/signature/3 branch from e81c85a to 22a4bd9CompareApril 23, 2025 22:34
@tarcieri

Copy link
Copy Markdown
Member

Ugh, that’s really annoying if the blanket impl leads to an inference failure.

Regarding carrying the digest, perhaps something like https://docs.rs/ecdsa/latest/ecdsa/struct.SignatureWithOid.html ?

@tarcieri

Copy link
Copy Markdown
Member

I opened RustCrypto/traits#1831 to discuss the inference regression. We might want to consider backing out this change.

@baloo

baloo commented Apr 24, 2025

Copy link
Copy Markdown
MemberAuthor

I'm not sure I get what you're trying to suggest with SignatureWithOid.

I think this overall is a side effect of having to provide the Digest to the blanket Signer, which you can't provide unless you specify that through the resulting type. (Because there could be more than one DigestSigner implemented).

I otherwise think that specifying the digest attached to a signature makes sense (and that we should actually embrace that change, whether or not we fix the inference for it).

@tarcieri

Copy link
Copy Markdown
Member

One thing that would be weird about attaching a digest type to the signature type is algorithm information isn't available in the serialized signature, so you'll be able to parse signatures generated by other digest algorithms successfully

@baloo

baloo commented Apr 24, 2025

Copy link
Copy Markdown
MemberAuthor

Yeah but that should be inferred from the VerifierDigest call that should ensue (otherwise the digest can be whatever)

Note: Neither the ecdsa::signature nor ecdsa::der::Signature have it either they don’t have digest in the first place

@baloo

Copy link
Copy Markdown
MemberAuthor

Wait, we do have the digest in the SigningKey, there should not be an issue with selecting the DigestSigner. I’ll have another look tomorrow.

@baloo

Copy link
Copy Markdown
MemberAuthor

#505 (comment)
That was actually a red herring, I didn't have PrehashSignature for Signature in the first place, so I could not use the blanket. The only way to implement PrehashSignature would here be to have signature carry the associated digest (which I did).
The type inference works just fine after that.

We get away with not having the Digest in ecdsa::Signature because the associated Digest is used instead.

#[derive(Debug, Clone, PartialEq, Eq)]
pub struct Signature {
#[derive(Eq)]
pub struct Signature<D> {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This still seems a little weird to me because there isn't a strong binding or relationship between the signature as a cryptographic object and the digest algorithm that was used to compute it.

This means that the type does not actually maintain an invariant e.g. "this is a signature that was known to be computed by using digest D over the input message". It could've been computed with any digest algorithm.

I guess there's a type safety argument to it in that the type identifies what digest you're supposed to use, but as cryptographic objects they're not really parameterized/distinguished by the digest, it's just something that happens earlier in the computation of the signature.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@baloo WDYT?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I don’t know what I think ^^.
I know that I don’t really have much of a choice.

What I’m arguing it that it makes the digest explicit when accepting/parsing a signature: “I know this is going to be verified with this public key with sha384”

I know the serialization does not necessarily carry this information, although it would be carried via the OID on the object (x509 or cms) or via a specification or however the developer may which. Whichever that might be the digest will need to be provided for the VerifyingKey.

all in all this is a little bit inconvenient, but not all that much and it just makes the digest choice explicit.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess the only reason it exists is for the PrehashSignature impl.

I find it a little troubling it otherwise doesn't actually do anything, but I guess we need to merge this to make any progress.

Comment on lines -146 to -154
#[cfg(feature = "os_rng")]
impl<D> Signer<Signature> for SigningKey<D>
where
D: Digest + FixedOutputReset,
{
fn try_sign(&self, msg: &[u8]) -> signature::Result<Signature> {
self.try_sign_with_rng(&mut OsRng, msg)
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess the alternative would be to preserve these Signer impls and not impl PrehashSignature for Signature since it's not actually known at the type level

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Iirc I tried that, but I came in conflict with the blanket implementation.

can’t try it right this moment, will try that in an hour or so.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you don't impl PrehashSignature the blanket impl shouldn't matter

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

yeah looks like it works, but that still mess up the type inference and you need to provide the type of the signature you expect. You don't need to provide the digest thought.

#510

@baloobaloo mentioned this pull request Apr 30, 2025
@baloo

baloo commented May 2, 2025

Copy link
Copy Markdown
MemberAuthor

closed in favor of #513

@baloobaloo closed this May 2, 2025
@baloo
baloo deleted the baloo/signature/3 branch May 2, 2025 22:43
Sign up for freeto 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.

2 participants

@baloo@tarcieri
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

chore(deps): bump signature to 3.0.0-pre - #505

Closed
baloo wants to merge 1 commit into
RustCrypto:masterfrom
baloo:baloo/signature/3
Closed

chore(deps): bump signature to 3.0.0-pre#505
baloo wants to merge 1 commit into
RustCrypto:masterfrom
baloo:baloo/signature/3

Conversation

@baloo

Copy link
Copy Markdown
Member

No description provided.

@baloo
baloo marked this pull request as draft April 23, 2025 21:46
@baloo
balooforce-pushed the baloo/signature/3 branch 2 times, most recently from eaaea51 to e81c85aCompareApril 23, 2025 22:31
@baloo
baloo marked this pull request as ready for review April 23, 2025 22:32
@baloo
balooforce-pushed the baloo/signature/3 branch from e81c85a to 22a4bd9CompareApril 23, 2025 22:34
@tarcieri

Copy link
Copy Markdown
Member

Ugh, that’s really annoying if the blanket impl leads to an inference failure.

Regarding carrying the digest, perhaps something like https://docs.rs/ecdsa/latest/ecdsa/struct.SignatureWithOid.html ?

@tarcieri

Copy link
Copy Markdown
Member

I opened RustCrypto/traits#1831 to discuss the inference regression. We might want to consider backing out this change.

@baloo

baloo commented Apr 24, 2025

Copy link
Copy Markdown
MemberAuthor

I'm not sure I get what you're trying to suggest with SignatureWithOid.

I think this overall is a side effect of having to provide the Digest to the blanket Signer, which you can't provide unless you specify that through the resulting type. (Because there could be more than one DigestSigner implemented).

I otherwise think that specifying the digest attached to a signature makes sense (and that we should actually embrace that change, whether or not we fix the inference for it).

@tarcieri

Copy link
Copy Markdown
Member

One thing that would be weird about attaching a digest type to the signature type is algorithm information isn't available in the serialized signature, so you'll be able to parse signatures generated by other digest algorithms successfully

@baloo

baloo commented Apr 24, 2025

Copy link
Copy Markdown
MemberAuthor

Yeah but that should be inferred from the VerifierDigest call that should ensue (otherwise the digest can be whatever)

Note: Neither the ecdsa::signature nor ecdsa::der::Signature have it either they don’t have digest in the first place

@baloo

Copy link
Copy Markdown
MemberAuthor

Wait, we do have the digest in the SigningKey, there should not be an issue with selecting the DigestSigner. I’ll have another look tomorrow.

@baloo

Copy link
Copy Markdown
MemberAuthor

#505 (comment)
That was actually a red herring, I didn't have PrehashSignature for Signature in the first place, so I could not use the blanket. The only way to implement PrehashSignature would here be to have signature carry the associated digest (which I did).
The type inference works just fine after that.

We get away with not having the Digest in ecdsa::Signature because the associated Digest is used instead.

#[derive(Debug, Clone, PartialEq, Eq)]
pub struct Signature {
#[derive(Eq)]
pub struct Signature<D> {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This still seems a little weird to me because there isn't a strong binding or relationship between the signature as a cryptographic object and the digest algorithm that was used to compute it.

This means that the type does not actually maintain an invariant e.g. "this is a signature that was known to be computed by using digest D over the input message". It could've been computed with any digest algorithm.

I guess there's a type safety argument to it in that the type identifies what digest you're supposed to use, but as cryptographic objects they're not really parameterized/distinguished by the digest, it's just something that happens earlier in the computation of the signature.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@baloo WDYT?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I don’t know what I think ^^.
I know that I don’t really have much of a choice.

What I’m arguing it that it makes the digest explicit when accepting/parsing a signature: “I know this is going to be verified with this public key with sha384”

I know the serialization does not necessarily carry this information, although it would be carried via the OID on the object (x509 or cms) or via a specification or however the developer may which. Whichever that might be the digest will need to be provided for the VerifyingKey.

all in all this is a little bit inconvenient, but not all that much and it just makes the digest choice explicit.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess the only reason it exists is for the PrehashSignature impl.

I find it a little troubling it otherwise doesn't actually do anything, but I guess we need to merge this to make any progress.

Comment on lines -146 to -154
#[cfg(feature = "os_rng")]
impl<D> Signer<Signature> for SigningKey<D>
where
D: Digest + FixedOutputReset,
{
fn try_sign(&self, msg: &[u8]) -> signature::Result<Signature> {
self.try_sign_with_rng(&mut OsRng, msg)
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess the alternative would be to preserve these Signer impls and not impl PrehashSignature for Signature since it's not actually known at the type level

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Iirc I tried that, but I came in conflict with the blanket implementation.

can’t try it right this moment, will try that in an hour or so.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you don't impl PrehashSignature the blanket impl shouldn't matter

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

yeah looks like it works, but that still mess up the type inference and you need to provide the type of the signature you expect. You don't need to provide the digest thought.

#510

@baloobaloo mentioned this pull request Apr 30, 2025
@baloo

baloo commented May 2, 2025

Copy link
Copy Markdown
MemberAuthor

closed in favor of #513

@baloobaloo closed this May 2, 2025
@baloo
baloo deleted the baloo/signature/3 branch May 2, 2025 22:43
Sign up for freeto 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.

2 participants

@baloo@tarcieri
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

chore(deps): bump signature to 3.0.0-pre - #505

Closed
baloo wants to merge 1 commit into
RustCrypto:masterfrom
baloo:baloo/signature/3
Closed

chore(deps): bump signature to 3.0.0-pre#505
baloo wants to merge 1 commit into
RustCrypto:masterfrom
baloo:baloo/signature/3

Conversation

@baloo

Copy link
Copy Markdown
Member

No description provided.

@baloo
baloo marked this pull request as draft April 23, 2025 21:46
@baloo
balooforce-pushed the baloo/signature/3 branch 2 times, most recently from eaaea51 to e81c85aCompareApril 23, 2025 22:31
@baloo
baloo marked this pull request as ready for review April 23, 2025 22:32
@baloo
balooforce-pushed the baloo/signature/3 branch from e81c85a to 22a4bd9CompareApril 23, 2025 22:34
@tarcieri

Copy link
Copy Markdown
Member

Ugh, that’s really annoying if the blanket impl leads to an inference failure.

Regarding carrying the digest, perhaps something like https://docs.rs/ecdsa/latest/ecdsa/struct.SignatureWithOid.html ?

@tarcieri

Copy link
Copy Markdown
Member

I opened RustCrypto/traits#1831 to discuss the inference regression. We might want to consider backing out this change.

@baloo

baloo commented Apr 24, 2025

Copy link
Copy Markdown
MemberAuthor

I'm not sure I get what you're trying to suggest with SignatureWithOid.

I think this overall is a side effect of having to provide the Digest to the blanket Signer, which you can't provide unless you specify that through the resulting type. (Because there could be more than one DigestSigner implemented).

I otherwise think that specifying the digest attached to a signature makes sense (and that we should actually embrace that change, whether or not we fix the inference for it).

@tarcieri

Copy link
Copy Markdown
Member

One thing that would be weird about attaching a digest type to the signature type is algorithm information isn't available in the serialized signature, so you'll be able to parse signatures generated by other digest algorithms successfully

@baloo

baloo commented Apr 24, 2025

Copy link
Copy Markdown
MemberAuthor

Yeah but that should be inferred from the VerifierDigest call that should ensue (otherwise the digest can be whatever)

Note: Neither the ecdsa::signature nor ecdsa::der::Signature have it either they don’t have digest in the first place

@baloo

Copy link
Copy Markdown
MemberAuthor

Wait, we do have the digest in the SigningKey, there should not be an issue with selecting the DigestSigner. I’ll have another look tomorrow.

@baloo

Copy link
Copy Markdown
MemberAuthor

#505 (comment)
That was actually a red herring, I didn't have PrehashSignature for Signature in the first place, so I could not use the blanket. The only way to implement PrehashSignature would here be to have signature carry the associated digest (which I did).
The type inference works just fine after that.

We get away with not having the Digest in ecdsa::Signature because the associated Digest is used instead.

#[derive(Debug, Clone, PartialEq, Eq)]
pub struct Signature {
#[derive(Eq)]
pub struct Signature<D> {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This still seems a little weird to me because there isn't a strong binding or relationship between the signature as a cryptographic object and the digest algorithm that was used to compute it.

This means that the type does not actually maintain an invariant e.g. "this is a signature that was known to be computed by using digest D over the input message". It could've been computed with any digest algorithm.

I guess there's a type safety argument to it in that the type identifies what digest you're supposed to use, but as cryptographic objects they're not really parameterized/distinguished by the digest, it's just something that happens earlier in the computation of the signature.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@baloo WDYT?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I don’t know what I think ^^.
I know that I don’t really have much of a choice.

What I’m arguing it that it makes the digest explicit when accepting/parsing a signature: “I know this is going to be verified with this public key with sha384”

I know the serialization does not necessarily carry this information, although it would be carried via the OID on the object (x509 or cms) or via a specification or however the developer may which. Whichever that might be the digest will need to be provided for the VerifyingKey.

all in all this is a little bit inconvenient, but not all that much and it just makes the digest choice explicit.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess the only reason it exists is for the PrehashSignature impl.

I find it a little troubling it otherwise doesn't actually do anything, but I guess we need to merge this to make any progress.

Comment on lines -146 to -154
#[cfg(feature = "os_rng")]
impl<D> Signer<Signature> for SigningKey<D>
where
D: Digest + FixedOutputReset,
{
fn try_sign(&self, msg: &[u8]) -> signature::Result<Signature> {
self.try_sign_with_rng(&mut OsRng, msg)
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess the alternative would be to preserve these Signer impls and not impl PrehashSignature for Signature since it's not actually known at the type level

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Iirc I tried that, but I came in conflict with the blanket implementation.

can’t try it right this moment, will try that in an hour or so.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you don't impl PrehashSignature the blanket impl shouldn't matter

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

yeah looks like it works, but that still mess up the type inference and you need to provide the type of the signature you expect. You don't need to provide the digest thought.

#510

@baloobaloo mentioned this pull request Apr 30, 2025
@baloo

baloo commented May 2, 2025

Copy link
Copy Markdown
MemberAuthor

closed in favor of #513

@baloobaloo closed this May 2, 2025
@baloo
baloo deleted the baloo/signature/3 branch May 2, 2025 22:43
Sign up for freeto 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.

2 participants

@baloo@tarcieri
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

chore(deps): bump signature to 3.0.0-pre - #505

Closed
baloo wants to merge 1 commit into
RustCrypto:masterfrom
baloo:baloo/signature/3
Closed

chore(deps): bump signature to 3.0.0-pre#505
baloo wants to merge 1 commit into
RustCrypto:masterfrom
baloo:baloo/signature/3

Conversation

@baloo

Copy link
Copy Markdown
Member

No description provided.

@baloo
baloo marked this pull request as draft April 23, 2025 21:46
@baloo
balooforce-pushed the baloo/signature/3 branch 2 times, most recently from eaaea51 to e81c85aCompareApril 23, 2025 22:31
@baloo
baloo marked this pull request as ready for review April 23, 2025 22:32
@baloo
balooforce-pushed the baloo/signature/3 branch from e81c85a to 22a4bd9CompareApril 23, 2025 22:34
@tarcieri

Copy link
Copy Markdown
Member

Ugh, that’s really annoying if the blanket impl leads to an inference failure.

Regarding carrying the digest, perhaps something like https://docs.rs/ecdsa/latest/ecdsa/struct.SignatureWithOid.html ?

@tarcieri

Copy link
Copy Markdown
Member

I opened RustCrypto/traits#1831 to discuss the inference regression. We might want to consider backing out this change.

@baloo

baloo commented Apr 24, 2025

Copy link
Copy Markdown
MemberAuthor

I'm not sure I get what you're trying to suggest with SignatureWithOid.

I think this overall is a side effect of having to provide the Digest to the blanket Signer, which you can't provide unless you specify that through the resulting type. (Because there could be more than one DigestSigner implemented).

I otherwise think that specifying the digest attached to a signature makes sense (and that we should actually embrace that change, whether or not we fix the inference for it).

@tarcieri

Copy link
Copy Markdown
Member

One thing that would be weird about attaching a digest type to the signature type is algorithm information isn't available in the serialized signature, so you'll be able to parse signatures generated by other digest algorithms successfully

@baloo

baloo commented Apr 24, 2025

Copy link
Copy Markdown
MemberAuthor

Yeah but that should be inferred from the VerifierDigest call that should ensue (otherwise the digest can be whatever)

Note: Neither the ecdsa::signature nor ecdsa::der::Signature have it either they don’t have digest in the first place

@baloo

Copy link
Copy Markdown
MemberAuthor

Wait, we do have the digest in the SigningKey, there should not be an issue with selecting the DigestSigner. I’ll have another look tomorrow.

@baloo

Copy link
Copy Markdown
MemberAuthor

#505 (comment)
That was actually a red herring, I didn't have PrehashSignature for Signature in the first place, so I could not use the blanket. The only way to implement PrehashSignature would here be to have signature carry the associated digest (which I did).
The type inference works just fine after that.

We get away with not having the Digest in ecdsa::Signature because the associated Digest is used instead.

#[derive(Debug, Clone, PartialEq, Eq)]
pub struct Signature {
#[derive(Eq)]
pub struct Signature<D> {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This still seems a little weird to me because there isn't a strong binding or relationship between the signature as a cryptographic object and the digest algorithm that was used to compute it.

This means that the type does not actually maintain an invariant e.g. "this is a signature that was known to be computed by using digest D over the input message". It could've been computed with any digest algorithm.

I guess there's a type safety argument to it in that the type identifies what digest you're supposed to use, but as cryptographic objects they're not really parameterized/distinguished by the digest, it's just something that happens earlier in the computation of the signature.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@baloo WDYT?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I don’t know what I think ^^.
I know that I don’t really have much of a choice.

What I’m arguing it that it makes the digest explicit when accepting/parsing a signature: “I know this is going to be verified with this public key with sha384”

I know the serialization does not necessarily carry this information, although it would be carried via the OID on the object (x509 or cms) or via a specification or however the developer may which. Whichever that might be the digest will need to be provided for the VerifyingKey.

all in all this is a little bit inconvenient, but not all that much and it just makes the digest choice explicit.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess the only reason it exists is for the PrehashSignature impl.

I find it a little troubling it otherwise doesn't actually do anything, but I guess we need to merge this to make any progress.

Comment on lines -146 to -154
#[cfg(feature = "os_rng")]
impl<D> Signer<Signature> for SigningKey<D>
where
D: Digest + FixedOutputReset,
{
fn try_sign(&self, msg: &[u8]) -> signature::Result<Signature> {
self.try_sign_with_rng(&mut OsRng, msg)
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess the alternative would be to preserve these Signer impls and not impl PrehashSignature for Signature since it's not actually known at the type level

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Iirc I tried that, but I came in conflict with the blanket implementation.

can’t try it right this moment, will try that in an hour or so.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you don't impl PrehashSignature the blanket impl shouldn't matter

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

yeah looks like it works, but that still mess up the type inference and you need to provide the type of the signature you expect. You don't need to provide the digest thought.

#510

@baloobaloo mentioned this pull request Apr 30, 2025
@baloo

baloo commented May 2, 2025

Copy link
Copy Markdown
MemberAuthor

closed in favor of #513

@baloobaloo closed this May 2, 2025
@baloo
baloo deleted the baloo/signature/3 branch May 2, 2025 22:43
Sign up for freeto 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.

2 participants

@baloo@tarcieri