[webauthn] Refactor CMTG key fetching into helper [chromium/src : main]

0 views
Skip to first unread message

Nina Satragno (Gerrit)

unread,
Jul 7, 2026, 5:36:04 PM (2 days ago) Jul 7
to Ken Buchanan, Chromium LUCI CQ, chromium...@chromium.org, derinel+wat...@google.com, webauthn...@chromium.org
Attention needed from Ken Buchanan

Nina Satragno voted and added 1 comment

Votes added by Nina Satragno

Auto-Submit+1

1 comment

Patchset-level comments
File-level comment, Patchset 4 (Latest):
Nina Satragno . resolved

Ken, please take a look. Thank you! ^_^

Open in Gerrit

Related details

Attention is currently required from:
  • Ken Buchanan
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement satisfiedCode-Owners
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I36544743dcdca38778fff7acadc563deacabbb36
Gerrit-Change-Number: 8057807
Gerrit-PatchSet: 4
Gerrit-Owner: Nina Satragno <nsat...@chromium.org>
Gerrit-Reviewer: Ken Buchanan <ke...@chromium.org>
Gerrit-Reviewer: Nina Satragno <nsat...@chromium.org>
Gerrit-Attention: Ken Buchanan <ke...@chromium.org>
Gerrit-Comment-Date: Tue, 07 Jul 2026 21:35:49 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
satisfied_requirement
unsatisfied_requirement
open
diffy

Ken Buchanan (Gerrit)

unread,
Jul 8, 2026, 8:26:26 AM (yesterday) Jul 8
to Nina Satragno, Chromium LUCI CQ, chromium...@chromium.org, derinel+wat...@google.com, webauthn...@chromium.org
Attention needed from Nina Satragno

Ken Buchanan added 3 comments

File chrome/browser/webauthn/cmtg_key_fetcher.cc
Line 32, Patchset 4 (Latest): CHECK(!is_ready_);
Ken Buchanan . unresolved

Is there a reason to not hairpin the callback if `is_ready_` is already set? I get that the enclave controller won't do that but I'm asking if it doing so would be bad in some way.

File chrome/browser/webauthn/gpm_enclave_controller.cc
Line 1388, Patchset 4 (Latest): if (cmtg_key_fetcher_ && !cmtg_key_fetcher_->is_ready()) {
Ken Buchanan . unresolved

If this method is called a second time before the keys are ready, it will cause a crash on the `CHECK(!callback)` in `WaitForKeys`. We might want to handle that.

Line 1404, Patchset 4 (Latest): cmtg_key_fetcher_.reset();
Ken Buchanan . unresolved

This means we no longer cache keys across retries. Is that intended?

Open in Gerrit

Related details

Attention is currently required from:
  • Nina Satragno
Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Owners
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    • requirement is not satisfiedReview-Enforcement
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I36544743dcdca38778fff7acadc563deacabbb36
    Gerrit-Change-Number: 8057807
    Gerrit-PatchSet: 4
    Gerrit-Owner: Nina Satragno <nsat...@chromium.org>
    Gerrit-Reviewer: Ken Buchanan <ke...@chromium.org>
    Gerrit-Reviewer: Nina Satragno <nsat...@chromium.org>
    Gerrit-Attention: Nina Satragno <nsat...@chromium.org>
    Gerrit-Comment-Date: Wed, 08 Jul 2026 12:26:12 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Nina Satragno (Gerrit)

    unread,
    11:22 AM (2 hours ago) 11:22 AM
    to Ken Buchanan, Chromium LUCI CQ, chromium...@chromium.org, derinel+wat...@google.com, webauthn...@chromium.org
    Attention needed from Ken Buchanan

    Nina Satragno voted and added 3 comments

    Votes added by Nina Satragno

    Auto-Submit+1
    Commit-Queue+1

    3 comments

    File chrome/browser/webauthn/cmtg_key_fetcher.cc
    Line 32, Patchset 4: CHECK(!is_ready_);
    Ken Buchanan . resolved

    Is there a reason to not hairpin the callback if `is_ready_` is already set? I get that the enclave controller won't do that but I'm asking if it doing so would be bad in some way.

    Nina Satragno

    I don't think there's anything wrong and this probably makes it easier to use the API. Done.

    File chrome/browser/webauthn/gpm_enclave_controller.cc
    Line 1388, Patchset 4: if (cmtg_key_fetcher_ && !cmtg_key_fetcher_->is_ready()) {
    Ken Buchanan . resolved

    If this method is called a second time before the keys are ready, it will cause a crash on the `CHECK(!callback)` in `WaitForKeys`. We might want to handle that.

    Nina Satragno

    I think if this happened it would be a bug and can't think of a way to trigger this normally, but the state machine is so complicated I agree we should handle it. Done.

    Line 1404, Patchset 4: cmtg_key_fetcher_.reset();
    Ken Buchanan . resolved

    This means we no longer cache keys across retries. Is that intended?

    Nina Satragno

    This seems like a mistake, since if the request is restarted we don't get a new `GPMEnclaveController` instance I believe. Removed this line.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Ken Buchanan
    Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement satisfiedCode-Owners
      • requirement is not satisfiedCode-Review
      • requirement is not satisfiedReview-Enforcement
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: I36544743dcdca38778fff7acadc563deacabbb36
      Gerrit-Change-Number: 8057807
      Gerrit-PatchSet: 5
      Gerrit-Owner: Nina Satragno <nsat...@chromium.org>
      Gerrit-Reviewer: Ken Buchanan <ke...@chromium.org>
      Gerrit-Reviewer: Nina Satragno <nsat...@chromium.org>
      Gerrit-Attention: Ken Buchanan <ke...@chromium.org>
      Gerrit-Comment-Date: Thu, 09 Jul 2026 15:22:10 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: Yes
      Comment-In-Reply-To: Ken Buchanan <ke...@chromium.org>
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Ken Buchanan (Gerrit)

      unread,
      12:06 PM (1 hour ago) 12:06 PM
      to Nina Satragno, Chromium LUCI CQ, chromium...@chromium.org, derinel+wat...@google.com, webauthn...@chromium.org
      Attention needed from Nina Satragno

      Ken Buchanan voted and added 2 comments

      Votes added by Ken Buchanan

      Code-Review+1

      2 comments

      Patchset-level comments
      File-level comment, Patchset 6 (Latest):
      Ken Buchanan . resolved

      lgtm

      File chrome/browser/webauthn/cmtg_key_fetcher.h
      Line 51, Patchset 6 (Latest): // when keys are ready. Should only be called if is_ready() is false.
      Ken Buchanan . unresolved

      This sentence can be removed now.

      Open in Gerrit

      Related details

      Attention is currently required from:
      • Nina Satragno
      Submit Requirements:
        • requirement satisfiedCode-Coverage
        • requirement satisfiedCode-Owners
        • requirement satisfiedCode-Review
        • requirement is not satisfiedNo-Unresolved-Comments
        • requirement satisfiedReview-Enforcement
        Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
        Gerrit-MessageType: comment
        Gerrit-Project: chromium/src
        Gerrit-Branch: main
        Gerrit-Change-Id: I36544743dcdca38778fff7acadc563deacabbb36
        Gerrit-Change-Number: 8057807
        Gerrit-PatchSet: 6
        Gerrit-Owner: Nina Satragno <nsat...@chromium.org>
        Gerrit-Reviewer: Ken Buchanan <ke...@chromium.org>
        Gerrit-Reviewer: Nina Satragno <nsat...@chromium.org>
        Gerrit-Attention: Nina Satragno <nsat...@chromium.org>
        Gerrit-Comment-Date: Thu, 09 Jul 2026 16:06:30 +0000
        Gerrit-HasComments: Yes
        Gerrit-Has-Labels: Yes
        satisfied_requirement
        unsatisfied_requirement
        open
        diffy

        Nina Satragno (Gerrit)

        unread,
        12:13 PM (1 hour ago) 12:13 PM
        to Ken Buchanan, Chromium LUCI CQ, chromium...@chromium.org, derinel+wat...@google.com, webauthn...@chromium.org

        Nina Satragno voted and added 1 comment

        Votes added by Nina Satragno

        Auto-Submit+1
        Commit-Queue+2

        1 comment

        File chrome/browser/webauthn/cmtg_key_fetcher.h
        Line 51, Patchset 6: // when keys are ready. Should only be called if is_ready() is false.
        Ken Buchanan . resolved

        This sentence can be removed now.

        Nina Satragno

        Done

        Open in Gerrit

        Related details

        Attention set is empty
        Submit Requirements:
          • requirement satisfiedCode-Coverage
          • requirement satisfiedCode-Owners
          • requirement satisfiedCode-Review
          • requirement satisfiedReview-Enforcement
          Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
          Gerrit-MessageType: comment
          Gerrit-Project: chromium/src
          Gerrit-Branch: main
          Gerrit-Change-Id: I36544743dcdca38778fff7acadc563deacabbb36
          Gerrit-Change-Number: 8057807
          Gerrit-PatchSet: 7
          Gerrit-Owner: Nina Satragno <nsat...@chromium.org>
          Gerrit-Reviewer: Ken Buchanan <ke...@chromium.org>
          Gerrit-Reviewer: Nina Satragno <nsat...@chromium.org>
          Gerrit-Comment-Date: Thu, 09 Jul 2026 16:13:31 +0000
          Gerrit-HasComments: Yes
          Gerrit-Has-Labels: Yes
          Comment-In-Reply-To: Ken Buchanan <ke...@chromium.org>
          satisfied_requirement
          open
          diffy

          Chromium LUCI CQ (Gerrit)

          unread,
          1:16 PM (18 minutes ago) 1:16 PM
          to Nina Satragno, Ken Buchanan, chromium...@chromium.org, derinel+wat...@google.com, webauthn...@chromium.org

          Chromium LUCI CQ submitted the change with unreviewed changes

          Unreviewed changes

          6 is the latest approved patch-set.
          The change was submitted with unreviewed changes in the following files:

          ```
          The name of the file: chrome/browser/webauthn/cmtg_key_fetcher.h
          Insertions: 1, Deletions: 1.

          @@ -48,7 +48,7 @@
             std::optional<std::vector<std::vector<uint8_t>>> keys() { return keys_; }
           
             // Starts the blocking latency timer and registers a callback to be invoked
          -  // when keys are ready. Should only be called if is_ready() is false.
          +  // when keys are ready.
             void WaitForKeys(Callback callback);
           
            private:
          ```

          Change information

          Commit message:
          [webauthn] Refactor CMTG key fetching into helper

          Extract the CMTG (Credential Manager Trust Group) device key fetching,
          timeout, and blocking latency recording logic from
          GPMEnclaveController into a new CmtgKeyFetcher helper class.

          This refactoring simplifies GPMEnclaveController and makes the CMTG
          fetching logic reusable for other controllers.

          TAG=agy
          CONV=22d5e913-d277-45fc-a0c7-8ebfce14e2e1
          Bug: 485888879
          Change-Id: I36544743dcdca38778fff7acadc563deacabbb36
          Auto-Submit: Nina Satragno <nsat...@chromium.org>
          Commit-Queue: Nina Satragno <nsat...@chromium.org>
          Reviewed-by: Ken Buchanan <ke...@chromium.org>
          Cr-Commit-Position: refs/heads/main@{#1659663}
          Files:
          • M chrome/browser/webauthn/BUILD.gn
          • A chrome/browser/webauthn/cmtg_key_fetcher.cc
          • A chrome/browser/webauthn/cmtg_key_fetcher.h
          • A chrome/browser/webauthn/cmtg_key_fetcher_unittest.cc
          • M chrome/browser/webauthn/gpm_enclave_controller.cc
          • M chrome/browser/webauthn/gpm_enclave_controller.h
          • M components/webauthn/core/browser/fake_cmtg_device_key_provider.cc
          • M components/webauthn/core/browser/fake_cmtg_device_key_provider.h
          Change size: L
          Delta: 8 files changed, 330 insertions(+), 76 deletions(-)
          Branch: refs/heads/main
          Submit Requirements:
          • requirement satisfiedCode-Review: +1 by Ken Buchanan
          Open in Gerrit
          Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
          Gerrit-MessageType: merged
          Gerrit-Project: chromium/src
          Gerrit-Branch: main
          Gerrit-Change-Id: I36544743dcdca38778fff7acadc563deacabbb36
          Gerrit-Change-Number: 8057807
          Gerrit-PatchSet: 8
          Gerrit-Owner: Nina Satragno <nsat...@chromium.org>
          Gerrit-Reviewer: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>
          Gerrit-Reviewer: Ken Buchanan <ke...@chromium.org>
          Gerrit-Reviewer: Nina Satragno <nsat...@chromium.org>
          open
          diffy
          satisfied_requirement
          Reply all
          Reply to author
          Forward
          0 new messages