| Message ID | 20211022150949.1754477-2-fs@gigacodes.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ssh signing: verify key lifetime | expand |
Fabian Stelzer <fs@gigacodes.de> writes: > Adds two new parameters to the check_signature api and passes them to the "Add". "pass". > internal verification ssh/gpg methods. > A payload timestamp that will be used to verify signatures at the time of their > objects creation if the signing method supports it (only ssh for now). OK. > And a signer strbuf containing ident information about the signer that > we will need for implementing "Trust on first use" in a future patch > series. OK. It would flow better in our "git log" stream if you explain upfront what problem you are trying to solve, before starting to give orders to the codebase to pass these pieces of information. Something like: In order to implement the "trust on the first use of a key" behaviour in later steps, allow callers to optionally pass the timestamp of the payload and the identity of the signer to check_signature(). It is unclear what "payload timestamp" is without actually seeing how it is used, so if you cannot explain it in easy terms in the log message for this step, it may be an indication that it is not a such good idea to add these parameters in a separate step. > int check_signature(const char *payload, size_t plen, > - const char *signature, size_t slen, > - struct signature_check *sigc); > + timestamp_t payload_timestamp, > + struct strbuf *payload_signer, const char *signature, > + size_t slen, struct signature_check *sigc); Funny line wrapping. Just like payload and plen form a pair (hence they sit next to each other on the same line), signature and slen should sit next to each other on the same line. What's the reason why payload-signer MUST come in a strbuf? A caller that has only ptr and len must invent a new strbuf that is otherwise unused to call this function because of that calling convention, which looks suboptimal. If the function accepts <ptr, len>, just like <payload, plen> or <signature, slen> are taken by the function, such a caller can just call the function without having to have an extra instance of strbuf, while a caller that happens to already have a strbuf can pass <buf.buf, buf.len> as two separate parameters. What's the driving principle that decided where in the list of parameters these two new ones are added? I would explain one possible order I may find "logical" using the level of detail I expect from an answer to "what's the guiding principle?" as an example here: - we should move 'sigc' to the beginning, because the convention used by many of our API functions that have some central structure to work with is to have such a structure as the first thing in the list of parameters; - we should then append 'payload_timestamp', 'payload_signer', and 'payload_signer_len' at the end, as the function is about "validate <signature, slen> is valid for <payload, plen> and leave the result in <sigc>", and anything we add is auxiliary input to the process, which are of less significance than the existing ones. Another possible direction to go might be to add these auxiliary input to the process to the signature_check structure. Then this step can disappear, or just flip the order of the parameter to move sigc to the beginning, and then the later stemp that does the "first use" thing can add the necessary members to the structure *and* use the members to do its thing, which helps readers better understand what is going on. One possible downside is that sigc has been mostly output-only structure, and turning it into a structure that also has some input members might make it too confusing. I dunno. Thanks.
On 24.10.21 01:13, Junio C Hamano wrote: > Fabian Stelzer <fs@gigacodes.de> writes: > > It would flow better in our "git log" stream if you explain upfront > what problem you are trying to solve, before starting to give orders > to the codebase to pass these pieces of information. Something > like: > > In order to implement the "trust on the first use of a key" > behaviour in later steps, allow callers to optionally pass the > timestamp of the payload and the identity of the signer to > check_signature(). Thanks, you're right. I will update the commit message. > > It is unclear what "payload timestamp" is without actually seeing > how it is used, so if you cannot explain it in easy terms in the log > message for this step, it may be an indication that it is not a such > good idea to add these parameters in a separate step. > >> int check_signature(const char *payload, size_t plen, >> - const char *signature, size_t slen, >> - struct signature_check *sigc); >> + timestamp_t payload_timestamp, >> + struct strbuf *payload_signer, const char *signature, >> + size_t slen, struct signature_check *sigc); > > Funny line wrapping. Just like payload and plen form a pair (hence > they sit next to each other on the same line), signature and slen > should sit next to each other on the same line. clang-format enforced the 80 char limit here i think :/ i think we will change this function anyway (see below) and i will keep it in mind then. > > What's the reason why payload-signer MUST come in a strbuf? A > caller that has only ptr and len must invent a new strbuf that is > otherwise unused to call this function because of that calling > convention, which looks suboptimal. It does not necessarily need to be. I just like the convenience of the strbuf_ functions for working with strings. Also i think it makes it very specific and easy to grasp in the calling function to STRBUF_INIT and then release it at the end. When working with char* it can often be not immediately clear (at least not t ome) if i need to provide an existing buffer or just a pointer and if need to free what was returned. char*/const char* usually indicates this, but this is not always the case. Maybe my "C foo" is just not strong enough and i like the higher level api :D If my change proposed below is acceptable this problem solves itself as well. > > > What's the driving principle that decided where in the list of > parameters these two new ones are added? > > I would explain one possible order I may find "logical" using the > level of detail I expect from an answer to "what's the guiding > principle?" as an example here: > > - we should move 'sigc' to the beginning, because the convention > used by many of our API functions that have some central > structure to work with is to have such a structure as the first > thing in the list of parameters; > > - we should then append 'payload_timestamp', 'payload_signer', and > 'payload_signer_len' at the end, as the function is about > "validate <signature, slen> is valid for <payload, plen> and > leave the result in <sigc>", and anything we add is auxiliary > input to the process, which are of less significance than the > existing ones. > > Another possible direction to go might be to add these auxiliary > input to the process to the signature_check structure. Then this > step can disappear, or just flip the order of the parameter to move > sigc to the beginning, and then the later stemp that does the "first > use" thing can add the necessary members to the structure *and* use > the members to do its thing, which helps readers better understand > what is going > > One possible downside is that sigc has been mostly output-only > structure, and turning it into a structure that also has some input > members might make it too confusing. I dunno. When starting this series my plan was indeed to add these parameters to the signature_check struct. As you correctly mentioned it is "output only" at the moment. That's why i chose to add the extra parameters. I added them in the middle of the parameter list next to the payload itself since i consider them to be part of to the payload. And since sigc is output only i would put it at the end (not sure if any convention for things like this exist for git). To put it in words: check_signature uses payload/plen with timestamp & ident to check signature/slen and return the result via sigc. Or maybe alternatively: check_signature checks payload/plen to check signature/slen, constrained by timestamp & ident and return the result via sigc. However if everyone is ok with changing the struct to be used for input as well then i would adjust the function to have it as the first parameter. The sigc struct already has a payload member (only used for verbose output) which is populated by the check function with a xmemdupz. I would then change it to a const char, add the length var and use it to pass the payload into the function as well. This way we also avoid the unnecessary mem copy. The function would just become: check_signature(struct signature_check *sigc, const char *signature, size_t slen) > > Thanks. > Thanks for your review.
Fabian Stelzer <fs@gigacodes.de> writes: > On 24.10.21 01:13, Junio C Hamano wrote: >> >> One possible downside is that sigc has been mostly output-only >> structure, and turning it into a structure that also has some input >> members might make it too confusing. I dunno. > > However if everyone is ok with changing the struct to be used for input > as well then i would adjust the function to have it as the first parameter. > > The sigc struct already has a payload member (only used for verbose > output) which is populated by the check function with a xmemdupz. I > would then change it to a const char, add the length var and use it to > pass the payload into the function as well. This way we also avoid the > unnecessary mem copy. > > The function would just become: > check_signature(struct signature_check *sigc, const char *signature, > size_t slen) I do not offhand think of a huge downside going that route myself. We seem to end up with unusually large number of folks on the CC list for some reason, so hopefully somebody will stop us if it is a stupid idea ;-) Thanks.
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 49b846d960..761c70642b 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -769,8 +769,9 @@ static void prepare_push_cert_sha1(struct child_process *proc) memset(&sigcheck, '\0', sizeof(sigcheck)); bogs = parse_signed_buffer(push_cert.buf, push_cert.len); - check_signature(push_cert.buf, bogs, push_cert.buf + bogs, - push_cert.len - bogs, &sigcheck); + check_signature(push_cert.buf, bogs, 0, NULL, + push_cert.buf + bogs, push_cert.len - bogs, + &sigcheck); nonce_status = check_nonce(push_cert.buf, bogs); } diff --git a/commit.c b/commit.c index 551de4903c..1704d9df0a 100644 --- a/commit.c +++ b/commit.c @@ -1212,7 +1212,7 @@ int check_commit_signature(const struct commit *commit, struct signature_check * if (parse_signed_commit(commit, &payload, &signature, the_hash_algo) <= 0) goto out; - ret = check_signature(payload.buf, payload.len, signature.buf, + ret = check_signature(payload.buf, payload.len, 0, NULL, signature.buf, signature.len, sigc); out: diff --git a/fmt-merge-msg.c b/fmt-merge-msg.c index 5216191488..d2cedad6b7 100644 --- a/fmt-merge-msg.c +++ b/fmt-merge-msg.c @@ -533,8 +533,8 @@ static void fmt_merge_msg_sigs(struct strbuf *out) else { buf = payload.buf; len = payload.len; - if (check_signature(payload.buf, payload.len, sig.buf, - sig.len, &sigc) && + if (check_signature(payload.buf, payload.len, 0, NULL, + sig.buf, sig.len, &sigc) && !sigc.output) strbuf_addstr(&sig, "gpg verification failed.\n"); else diff --git a/gpg-interface.c b/gpg-interface.c index 800d8caa67..6049f7cbf7 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -20,7 +20,10 @@ struct gpg_format { const char **sigs; int (*verify_signed_buffer)(struct signature_check *sigc, struct gpg_format *fmt, const char *payload, - size_t payload_size, const char *signature, + size_t payload_size, + timestamp_t payload_timestamp, + struct strbuf *payload_signer, + const char *signature, size_t signature_size); int (*sign_buffer)(struct strbuf *buffer, struct strbuf *signature, const char *signing_key); @@ -54,11 +57,17 @@ static const char *ssh_sigs[] = { static int verify_gpg_signed_buffer(struct signature_check *sigc, struct gpg_format *fmt, const char *payload, - size_t payload_size, const char *signature, + size_t payload_size, + timestamp_t payload_timestamp, + struct strbuf *payload_signer, + const char *signature, size_t signature_size); static int verify_ssh_signed_buffer(struct signature_check *sigc, struct gpg_format *fmt, const char *payload, - size_t payload_size, const char *signature, + size_t payload_size, + timestamp_t payload_timestamp, + struct strbuf *payload_signer, + const char *signature, size_t signature_size); static int sign_buffer_gpg(struct strbuf *buffer, struct strbuf *signature, const char *signing_key); @@ -315,7 +324,10 @@ static void parse_gpg_output(struct signature_check *sigc) static int verify_gpg_signed_buffer(struct signature_check *sigc, struct gpg_format *fmt, const char *payload, - size_t payload_size, const char *signature, + size_t payload_size, + timestamp_t payload_timestamp, + struct strbuf *payload_signer, + const char *signature, size_t signature_size) { struct child_process gpg = CHILD_PROCESS_INIT; @@ -425,7 +437,10 @@ static void parse_ssh_output(struct signature_check *sigc) static int verify_ssh_signed_buffer(struct signature_check *sigc, struct gpg_format *fmt, const char *payload, - size_t payload_size, const char *signature, + size_t payload_size, + timestamp_t payload_timestamp, + struct strbuf *payload_signer, + const char *signature, size_t signature_size) { struct child_process ssh_keygen = CHILD_PROCESS_INIT; @@ -560,8 +575,10 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc, return ret; } -int check_signature(const char *payload, size_t plen, const char *signature, - size_t slen, struct signature_check *sigc) +int check_signature(const char *payload, size_t plen, + timestamp_t payload_timestamp, + struct strbuf *payload_signer, const char *signature, + size_t slen, struct signature_check *sigc) { struct gpg_format *fmt; int status; @@ -573,8 +590,9 @@ int check_signature(const char *payload, size_t plen, const char *signature, if (!fmt) die(_("bad/incompatible signature '%s'"), signature); - status = fmt->verify_signed_buffer(sigc, fmt, payload, plen, signature, - slen); + status = fmt->verify_signed_buffer(sigc, fmt, payload, plen, + payload_timestamp, payload_signer, + signature, slen); if (status && !sigc->output) return !!status; diff --git a/gpg-interface.h b/gpg-interface.h index beefacbb1e..f7c5389c90 100644 --- a/gpg-interface.h +++ b/gpg-interface.h @@ -71,8 +71,9 @@ const char *get_signing_key(void); */ const char *get_signing_key_id(void); int check_signature(const char *payload, size_t plen, - const char *signature, size_t slen, - struct signature_check *sigc); + timestamp_t payload_timestamp, + struct strbuf *payload_signer, const char *signature, + size_t slen, struct signature_check *sigc); void print_signature_buffer(const struct signature_check *sigc, unsigned flags); diff --git a/log-tree.c b/log-tree.c index 644893fd8c..3c3aec5c40 100644 --- a/log-tree.c +++ b/log-tree.c @@ -513,7 +513,7 @@ static void show_signature(struct rev_info *opt, struct commit *commit) if (parse_signed_commit(commit, &payload, &signature, the_hash_algo) <= 0) goto out; - status = check_signature(payload.buf, payload.len, signature.buf, + status = check_signature(payload.buf, payload.len, 0, NULL, signature.buf, signature.len, &sigc); if (status && !sigc.output) show_sig_lines(opt, status, "No signature\n"); @@ -583,7 +583,7 @@ static int show_one_mergetag(struct commit *commit, status = -1; if (parse_signature(extra->value, extra->len, &payload, &signature)) { /* could have a good signature */ - status = check_signature(payload.buf, payload.len, + status = check_signature(payload.buf, payload.len, 0, NULL, signature.buf, signature.len, &sigc); if (sigc.output) strbuf_addstr(&verify_message, sigc.output); diff --git a/tag.c b/tag.c index 3e18a41841..3459a0867c 100644 --- a/tag.c +++ b/tag.c @@ -25,7 +25,7 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags) return error("no signature found"); } - ret = check_signature(payload.buf, payload.len, signature.buf, + ret = check_signature(payload.buf, payload.len, 0, NULL, signature.buf, signature.len, &sigc); if (!(flags & GPG_VERIFY_OMIT_STATUS))
Adds two new parameters to the check_signature api and passes them to the internal verification ssh/gpg methods. A payload timestamp that will be used to verify signatures at the time of their objects creation if the signing method supports it (only ssh for now). And a signer strbuf containing ident information about the signer that we will need for implementing "Trust on first use" in a future patch series. Adding the ident info right now instead of a later patch series makes certain choices in this patch series clearer. Only passing the timestamp could be done a bit simpler on some call sites but i am certain that we will need the ident info for "Trust on first use" no matter how exactly it will be implemented later. To start with we pass 0, NULL on all invocations to keep the current behaviour as is. Signed-off-by: Fabian Stelzer <fs@gigacodes.de> --- builtin/receive-pack.c | 5 +++-- commit.c | 2 +- fmt-merge-msg.c | 4 ++-- gpg-interface.c | 36 +++++++++++++++++++++++++++--------- gpg-interface.h | 5 +++-- log-tree.c | 4 ++-- tag.c | 2 +- 7 files changed, 39 insertions(+), 19 deletions(-)