Skip to content

NuGetFeed: only encrypt in memory credential on supported platforms#1239

Merged
jamill merged 1 commit into
microsoft:masterfrom
jamill:upgrader_tasks/nuget_credentials
Jun 7, 2019
Merged

NuGetFeed: only encrypt in memory credential on supported platforms#1239
jamill merged 1 commit into
microsoft:masterfrom
jamill:upgrader_tasks/nuget_credentials

Conversation

@jamill
Copy link
Copy Markdown
Member

@jamill jamill commented Jun 5, 2019

The NuGet Client API has flags to control whether a password is stored
in clear text or not. This flag controls:

  1. Whether the password is stored in clear text when persisted to a
    config file on disk.

  2. Whether the password is stored encrypted in memory

Encrypting the password is only supported on Windows (and Mono)
platforms, and not on netcore platfroms.

As VFS for Git does not actually update any configuration files, the
flag only controls how the password is stored in memory. For netcore
platforms, do not set this flag. As VFS for Git does not persist this
data to disk, the tradeoff is whether the process is encrypting the
password in memory or not. VFS for Git itself is working with the
plain text password, so this is not broading the risk in this aspect.

@jamill jamill requested review from jrbriggs, nickgra and wilbaker June 5, 2019 19:16
@jamill
Copy link
Copy Markdown
Member Author

jamill commented Jun 5, 2019

I am open to feedback on this, including if I am going about this in the completely wrong way. One option is to just make the web requests ourselves and not use the NuGet client APIs.

@jamill jamill force-pushed the upgrader_tasks/nuget_credentials branch 2 times, most recently from 2d3cc55 to 1379521 Compare June 5, 2019 19:41
@jamill jamill mentioned this pull request Jun 5, 2019
15 tasks
Copy link
Copy Markdown
Member

@jrbriggs jrbriggs left a comment

Choose a reason for hiding this comment

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

This makes sense to me.

@nickgra
Copy link
Copy Markdown
Member

nickgra commented Jun 6, 2019

What kind of 'password' do we have in memory? Is this going to be the PAT that the GCM stores? Or are we going to have a flow where this could be an authorization token that is generated with a PAT (or via a GUI pop up dialog box like ADAL can create)? I would hope this isn't someone's actual AD credential :)

@jamill jamill force-pushed the upgrader_tasks/nuget_credentials branch from 1379521 to d3f0fb5 Compare June 6, 2019 03:03
@jamill
Copy link
Copy Markdown
Member Author

jamill commented Jun 6, 2019

What kind of 'password' do we have in memory? Is this going to be the PAT that the GCM stores? Or are we going to have a flow where this could be an authorization token that is generated with a PAT (or via a GUI pop up dialog box like ADAL can create)? I would hope this isn't someone's actual AD credential :)

This is going to be whatever credential the GCM returns. Currently, for the GCM that ships with VFS for Git, this is a PAT.

The NuGet Client API has flags to control whether a password is stored
in clear text or not. This flag controls:

  1) Whether the password is stored in clear text when persisted to a
  config file on disk.

  2) Whether the password is stored encrypted in memory

Encrypting the password is only supported on Windows (and Mono)
platforms, and not on netcore platfroms.

As VFS for Git does not actually update any configuration files, the
flag only controls how the password is stored in memory. For netcore
platforms, do not set this flag. As VFS for Git does not persist this
data to disk, the tradeoff is whether the process is encrypting the
password in memory or not. VFS for Git itself is working with the
plain text password, so this is not broading the risk in this aspect.
@jamill jamill force-pushed the upgrader_tasks/nuget_credentials branch from d3f0fb5 to bb677bb Compare June 7, 2019 13:26
@jamill jamill merged commit 4aaeb27 into microsoft:master Jun 7, 2019
@jamill jamill deleted the upgrader_tasks/nuget_credentials branch June 7, 2019 14:41
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.

3 participants