Skip to content

Conversation

mjcheetham
Copy link
Contributor

@mjcheetham mjcheetham commented Feb 22, 2023

Changes

Description PR(s)
Secret Service sec_free: Assertion cell->requested > 0 failed. error fix #996
Release pipeline fixes #995 #1009 #1061
Generic OAuth support via config 🎉 #1062
Initial TRACE2 tracing events #1045

Lessley Dennington and others added 30 commits December 14, 2022 16:31
When we began signing the .NET tool in 80cc677, we did not update the path
used for publishing to nuget.org. Fixing with this change.
When we began signing the .NET tool in 80cc677, we did not update the
path used for publishing to nuget.org. Fixing with this change.
Multiple Linux users have reported that they are unable to use Secret
Service as their credential store, as GCM throws the following error:

sec_free: Assertion `cell->requested > 0' failed.

The root cause is that we're using the libsecret secret_value_get()
function to obtain secret data, then attempting to free the string with
secret_password_free(). It appears that secret_password_free() is only
meant to be used to free nonpageable memory [1], however. Removing this
call fixes the issue, as verified with a successful git-credential-manager
diagnose (which was previously failing with the above error).

[1] secret_password_free manpage
https://www.manpagez.com/html/libsecret-1/libsecret-1-0.18.6/libsecret-Password-storage.php#secret-password-free
Multiple Linux users have reported that they are unable to use Secret
Service as their credential store, as GCM throws the following error:

sec_free: Assertion `cell->requested > 0' failed.

The root cause is that we're using the libsecret secret_value_get
function to obtain secret data, then attempting to free the string with
secret_password_free. It appears that secret_password_free is only meant
to be used to free nonpageable memory [1], however. Removing this call
fixes the issue, as verified with a successful git-credential-manager
diagnose (which was previously failing with the above error).

[1] secret_password_free manpage

https://www.manpagez.com/html/libsecret-1/libsecret-1-0.18.6/libsecret-Password-storage.php#secret-password-free

Fixes #793
At least in GitHub-flavored Markdown, that section title gets
transformed into the anchor `net-tool`, not `.NET-tool`.

Signed-off-by: Johannes Schindelin <[email protected]>
Something I noticed today, while looking for a way to install Git
Credential Manager for Windows/ARM64.
While we added PGP signatures for tarballs in 7baac73, we did not notice
that, while ESRP returns a file with the tar.gz extension, it is actually
the signature file, not the tarball itself. Correct with this change and
validate tarball moving forward so it doesn't happen again!
The GCM Debian package currently fails to install on Debian with the
following error:

error: archive 'gcm-linux_amd64.2.0.886.deb' uses unknown compression for
member 'control.tar.zst', giving up

It appears that the version of dpkg that ships with Debian does not
support zstd compression [1]. Enforcing xz compression resolves the issue.

This also provides an opportunity to clean up some unused variables in
pack.sh and ensure we check the architecture is found before attempting to
use the ARCH variable.

[1]: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=892664
While we added PGP signatures for tarballs in 7baac73, we did not notice
that, while ESRP returns a file with the tar.gz extension, it is actually
the signature file, not the tarball itself. Correct with this change and
validate tarball moving forward so it doesn't happen again!

Additionally, a user reported in #997 that the latest GCM Debian package
didn't work on Debian distributions. It appears that the version of dpkg
that ships with Debian [does not support zstd
compression](https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=892664).
Enforcing xz compression resolves the issue.
    
Finally, this provided an opportunity to clean up some unused variables
in pack.sh for Linux and to ensure we check the architecture is found
before attemping to use the ARCH variable.

These changes were validated with [this successful test
run](https://github.com/ldennington/git-credential-manager/actions/runs/3795232469)
in my fork.
Bumps [DavidAnson/markdownlint-cli2-action](https://github.com/DavidAnson/markdownlint-cli2-action) from 8.0.0 to 9.0.0.
- [Release notes](https://github.com/DavidAnson/markdownlint-cli2-action/releases)
- [Commits](DavidAnson/markdownlint-cli2-action@d57f8bd...5b7c9f7)

---
updated-dependencies:
- dependency-name: DavidAnson/markdownlint-cli2-action
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
Convert incorrect use of it's to its.
GCM's documentation states that git's credential cache is supported on
Windows. Unfortunately, due to lack of Unix socket support on Windows
versions prior to Windows 10, this feature is not currently supported on
Windows, so this change removes it from the list of platforms on which
this credstore can be used. See [1] for more details.

[1]: git-for-windows/git#3892
GCM's documentation states that git's credential cache is supported on
Windows. Unfortunately, due to lack of Unix socket support on Windows
versions prior to Windows 10, this feature is not currently supported on
Windows, so this change removes it from the list of platforms on which
this credstore can be used. See [this
issue](git-for-windows/git#3892) for more
details.
Fix a bug in a view model property for the basic credentials prompt; we
should be updating the backing field and also raising the
PropertyChanged event.
A user pointed out in a recent GCM issue that our symbols tarball does not
actually contain symbols (see [1] for additional details). Fix the
`pack.sh` script to package the correct files into the symbols tarball.

[1]: #1028 (comment)
A user pointed out in a recent GCM issue that our symbols tarball does not
actually contain symbols (see [1] for additional details). Fix the
`pack.sh` script to package the correct files into the symbols tarball.

[1]: #1028 (comment)
Teach the Generic host provider to read configuration for OAuth-based
authentication. These are largely parameters required for the
OAuth2Client to be constructed including Client ID/Secret, Redirect URI
and Scopes.
Add OAuth support for the generic provider offering browser (authcode
grant) and device code (device auth grant) support. Device code
and mode selection is initially only offered for TTY users.
Add support for storing and using OAuth refresh tokens.
Prepend "refresh_token" as a subdomain to give better chances of
avoiding a name clash compared with appending "/refresh_token" to the
path component.
mjcheetham and others added 23 commits February 1, 2023 11:09
Add shared view models and commands for the OAuth GUI prompts.

Two new commands and VMs are added, one for the initial 'mode'
selection, and another to display the device code.
Add AvaloniaUI based implementations of the OAuth and Device Code
generic UI prompts.
Add the WPF based GUI implementation of the OAuth and device code
generic prompts.
Add detection and use of GUI prompts for OAuth authentication.
Add ability to provide OAuth-based authentication for generic hosts by
way of simple Git configuration.

When a remote URL does not match any known host provider plugin, the
generic provider will now first check for OAuth configuration in the Git
config or environment variables. If such config is available then we try
and perform OAuth authentication. Support for device code flow is
optional, and refresh tokens will be used if the service supports and
returns them.

Users can make use of existing Git config `include` to easily organise
and share custom OAuth configurations.
The implementation of TRACE2 tracing will require use of the
TryGetAssemblyVersion method. To prepare for this, move this method out
of the DiagnoseCommand class and into its own static class.
The implementation of TRACE2 tracing will require truncation of long file
names just as TRACE does. To prepare for this, move this logic out of the
TRACE class and into its own method in a new static TraceUtils class.
Additionally, add a unit test to validate this logic.
Add initial TRACE2 functionality, including:

1. The ability to add writers (which will eventually write to Normal, Perf, and Event
format targets).
2. An abstract Trace2Message class.
3. Logic to send Trace2Messages to writers.
4. Ability to release writers prior to application exit.
Add the Trace2CollectorWriter class to accept Trace2 messages in the event target
format and write them to the OTel Collector/Telemetry Service.
Add Trace2StreamWriter class which will be used to write to stderr or a
file, based on the TextWriter that is passed to it. It will also adapt the
output format based on the format target that is passed to it.
A key component of Git's TRACE2 tracing system is the session id (sid).
This identifies the process instance to allow all events emitted by it to
be identified.

We check to see if a parent sid (i.e. from a Git process) exists. If so,
we separate this sid from the GCM sid (a GUID) using a "/". If there is no
parent sid, we simply generate a GUID for the GCM sid and use that.

The above also requires addition of a new SetEnvironmentVariable() method
in EnvironmentBase.cs to set the GCM-specific SID variable.
Add the infrastructure to detect whether a user has enabled the TRACE2
event format or normal format targets. This implementation has been
designed to be extended to include the perf format target in a future
series.

Additionally it involved some refactoring/cleanup to set the Application
Path and InstallationDirectory in the CommandContext, rather than in
GCM/UI helper Program.cs files.
Write the TRACE2 version event, which identifies the event format version
(currently hardcoded as 3 as it is in Git) and the version of GCM.

Additionally, write the TRACE2 start event, which reflects the elapsed time and
application arguments (including the application name). These are paired
because the telemetry tool/OTel collector require both these events to be
sent for a given session.
Add TRACE2 exit event, following the pattern established with the version
and start events.
Add initial `TRACE2` functionality, including:

1. Small refactorings to make `TryGetAssemblyVersion` accessible
outside of the `DiagnosticCommand` class and shared trace logic
accessible from a `TraceUtils` class.
2. Addition of a `Trace2CollectorWriter` class, which handles writing
to the Telemetry Tool/OTel collector and a `Trace2StreamWriter` class,
which handles writing to stderr/files.
3. Basic `TRACE2` functionality, including the ability to add writers to
different format targets, logic to send messages to these writers, and
the ability to release these writers before application exit.
4. Support for session IDs.
5. Ability for users to enable normal/event format targets.
6. Writing `Version`, `Start`, and `Exit` events.
Remove warning about being unable to set up tracing from Trace2
TryParseSettings method. This method should just check to see whether
TRACE2 is enabled - if it is not, it does not need to warn the user, as
we only collect traces from those who actively choose to opt in.
Fix an incorrectly-formatted Windows environment variable in rename.md.
GCM's UI helpers are connected to the TRACE2 system via the Start() call
in ApplicationBase.cs. Since this call records Version and Start
events, ensure a corresponding Exit event is called before the helper
process completes.
The TRACE2 convention for thread naming is for each child process's main
thread of execution to be named "main." In GCM, however, we encountered an
issue with our UI Helper exes - their main threads of execution are named
AppMain.

To temporarily work around this issue, we default the thread name for all
TRACE2 events to "main" (rather than changing GCM's process names). This
will do for our current events, which are all called from the main thread
of execution. However, it is important to note that future events (i.e.
regions) will require deeper thought around the GCM/TRACE2 process model,
as they will be called on threads from .NET's ThreadPool rather than the
main thread of execution.
Remove warning about being unable to set up tracing from Trace2
TryParseSettings method. This method should just check to see whether
TRACE2 is enabled - if it is not, it does not need to warn the user, as
we only collect traces from those who actively choose to opt in.

Additionally, GCM's UI helpers are connected to the TRACE2 system via
the Start() call in `ApplicationBase.cs`. Since this call records
`Version` and `Start` events, ensure a corresponding `Exit` event is
called before the helper process completes.
Copy link
Contributor

@ldennington ldennington left a comment

Choose a reason for hiding this comment

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

🎉

@ldennington ldennington merged commit f99d745 into release Feb 23, 2023
Sign up for free to 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.

7 participants