Skip to content

Commit 12d0b63

Browse files
TylerLeonhardtTylerLeonhardt
authored
more robust credentials service (microsoft#160725)
* more robust credentials service * use retry for any keytar action * add trace logging throughout * don't swallow errors when deleting chunks * more trace
1 parent 7a82e67 commit 12d0b63

File tree

2 files changed

+65
-60
lines changed

2 files changed

+65
-60
lines changed

‎src/vs/platform/credentials/common/credentialsMainService.ts

+61-60
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import { Emitter } from 'vs/base/common/event';
88
import { Disposable } from 'vs/base/common/lifecycle';
99
import { ILogService } from 'vs/platform/log/common/log';
1010
import { isWindows } from 'vs/base/common/platform';
11+
import { retry } from 'vs/base/common/async';
1112

1213
interface ChunkedPassword {
1314
content: string;
@@ -46,6 +47,7 @@ export abstract class BaseCredentialsMainService extends Disposable implements I
4647
//#endregion
4748

4849
async getPassword(service: string, account: string): Promise<string | null> {
50+
this.logService.trace('Getting password from keytar:', service, account);
4951
let keytar: KeytarModule;
5052
try {
5153
keytar = await this.withKeytar();
@@ -54,33 +56,37 @@ export abstract class BaseCredentialsMainService extends Disposable implements I
5456
return null;
5557
}
5658

57-
const password = await keytar.getPassword(service, account);
58-
if (password) {
59-
try {
60-
let { content, hasNextChunk }: ChunkedPassword = JSON.parse(password);
61-
if (!content || !hasNextChunk) {
62-
return password;
63-
}
64-
65-
let index = 1;
66-
while (hasNextChunk) {
67-
const nextChunk = await keytar.getPassword(service, `${account}-${index}`);
68-
const result: ChunkedPassword = JSON.parse(nextChunk!);
69-
content += result.content;
70-
hasNextChunk = result.hasNextChunk;
71-
index++;
72-
}
73-
74-
return content;
75-
} catch {
59+
const password = await retry(() => keytar.getPassword(service, account), 50, 3);
60+
if (!password) {
61+
this.logService.trace('Did not get a password from keytar for account:', account);
62+
return password;
63+
}
64+
try {
65+
let { content, hasNextChunk }: ChunkedPassword = JSON.parse(password);
66+
if (!content || !hasNextChunk) {
67+
this.logService.trace('Got password from keytar for account:', account);
7668
return password;
7769
}
78-
}
7970

80-
return password;
71+
let index = 1;
72+
while (hasNextChunk) {
73+
const nextChunk = await retry(() => keytar.getPassword(service, `${account}-${index}`), 50, 3);
74+
const result: ChunkedPassword = JSON.parse(nextChunk!);
75+
content += result.content;
76+
hasNextChunk = result.hasNextChunk;
77+
index++;
78+
}
79+
80+
this.logService.trace(`Got ${index}-chunked password from keytar for account:`, account);
81+
return content;
82+
} catch (e) {
83+
this.logService.error(e);
84+
return password;
85+
}
8186
}
8287

8388
async setPassword(service: string, account: string, password: string): Promise<void> {
89+
this.logService.trace('Setting password using keytar:', service, account);
8490
let keytar: KeytarModule;
8591
try {
8692
keytar = await this.withKeytar();
@@ -89,28 +95,6 @@ export abstract class BaseCredentialsMainService extends Disposable implements I
8995
throw e;
9096
}
9197

92-
const MAX_SET_ATTEMPTS = 3;
93-
94-
// Sometimes Keytar has a problem talking to the keychain on the OS. To be more resilient, we retry a few times.
95-
const setPasswordWithRetry = async (service: string, account: string, password: string) => {
96-
let attempts = 0;
97-
let error: any;
98-
while (attempts < MAX_SET_ATTEMPTS) {
99-
try {
100-
await keytar.setPassword(service, account, password);
101-
return;
102-
} catch (e) {
103-
error = e;
104-
this.logService.warn('Error attempting to set a password: ', e?.message ?? e);
105-
attempts++;
106-
await new Promise(resolve => setTimeout(resolve, 200));
107-
}
108-
}
109-
110-
// throw last error
111-
throw error;
112-
};
113-
11498
if (isWindows && password.length > BaseCredentialsMainService.MAX_PASSWORD_LENGTH) {
11599
let index = 0;
116100
let chunk = 0;
@@ -124,19 +108,21 @@ export abstract class BaseCredentialsMainService extends Disposable implements I
124108
content: passwordChunk,
125109
hasNextChunk: hasNextChunk
126110
};
127-
128-
await setPasswordWithRetry(service, chunk ? `${account}-${chunk}` : account, JSON.stringify(content));
111+
await retry(() => keytar.setPassword(service, chunk ? `${account}-${chunk}` : account, JSON.stringify(content)), 50, 3);
129112
chunk++;
130113
}
131114

115+
this.logService.trace(`Got${chunk ? ` ${chunk}-chunked` : ''} password from keytar for account:`, account);
132116
} else {
133-
await setPasswordWithRetry(service, account, password);
117+
await retry(() => keytar.setPassword(service, account, password), 50, 3);
118+
this.logService.trace('Got password from keytar for account:', account);
134119
}
135120

136121
this._onDidChangePassword.fire({ service, account });
137122
}
138123

139124
async deletePassword(service: string, account: string): Promise<boolean> {
125+
this.logService.trace('Deleting password using keytar:', service, account);
140126
let keytar: KeytarModule;
141127
try {
142128
keytar = await this.withKeytar();
@@ -147,14 +133,30 @@ export abstract class BaseCredentialsMainService extends Disposable implements I
147133

148134
const password = await keytar.getPassword(service, account);
149135
if (!password) {
136+
this.logService.trace('Did not get a password to delete from keytar for account:', account);
150137
return false;
151138
}
152-
const didDelete = await keytar.deletePassword(service, account);
139+
140+
let content: string | undefined;
141+
let hasNextChunk: boolean | undefined;
142+
try {
143+
const possibleChunk = JSON.parse(password);
144+
content = possibleChunk.content;
145+
hasNextChunk = possibleChunk.hasNextChunk;
146+
} catch {
147+
// When the password is saved the entire JSON payload is encrypted then stored, thus the result from getPassword might not be valid JSON
148+
// https://github.com/microsoft/vscode/blob/c22cb87311b5eb1a3bf5600d18733f7485355dc0/src/vs/workbench/api/browser/mainThreadSecretState.ts#L83
149+
// However in the chunked case we JSONify each chunk after encryption so for the chunked case we do expect valid JSON here
150+
// https://github.com/microsoft/vscode/blob/708cb0c507d656b760f9d08115b8ebaf8964fd73/src/vs/platform/credentials/common/credentialsMainService.ts#L128
151+
// Empty catch here just as in getPassword because we expect to handle both JSON cases and non JSON cases here it's not an error case to fail to parse
152+
// https://github.com/microsoft/vscode/blob/708cb0c507d656b760f9d08115b8ebaf8964fd73/src/vs/platform/credentials/common/credentialsMainService.ts#L76
153+
}
154+
155+
let index = 0;
153156
try {
154-
let { content, hasNextChunk }: ChunkedPassword = JSON.parse(password);
155157
if (content && hasNextChunk) {
156158
// need to delete additional chunks
157-
let index = 1;
159+
index++;
158160
while (hasNextChunk) {
159161
const accountWithIndex = `${account}-${index}`;
160162
const nextChunk = await keytar.getPassword(service, accountWithIndex);
@@ -165,20 +167,19 @@ export abstract class BaseCredentialsMainService extends Disposable implements I
165167
index++;
166168
}
167169
}
168-
} catch {
169-
// When the password is saved the entire JSON payload is encrypted then stored, thus the result from getPassword might not be valid JSON
170-
// https://github.com/microsoft/vscode/blob/c22cb87311b5eb1a3bf5600d18733f7485355dc0/src/vs/workbench/api/browser/mainThreadSecretState.ts#L83
171-
// However in the chunked case we JSONify each chunk after encryption so for the chunked case we do expect valid JSON here
172-
// https://github.com/microsoft/vscode/blob/708cb0c507d656b760f9d08115b8ebaf8964fd73/src/vs/platform/credentials/common/credentialsMainService.ts#L128
173-
// Empty catch here just as in getPassword because we expect to handle both JSON cases and non JSON cases here it's not an error case to fail to parse
174-
// https://github.com/microsoft/vscode/blob/708cb0c507d656b760f9d08115b8ebaf8964fd73/src/vs/platform/credentials/common/credentialsMainService.ts#L76
170+
} catch (e) {
171+
this.logService.error(e);
175172
}
176173

177-
if (didDelete) {
174+
// Delete the first account to determine deletion success
175+
if (await keytar.deletePassword(service, account)) {
178176
this._onDidChangePassword.fire({ service, account });
177+
this.logService.trace(`Deleted${index ? ` ${index}-chunked` : ''} password from keytar for account:`, account);
178+
return true;
179179
}
180180

181-
return didDelete;
181+
this.logService.trace(`Keytar failed to delete${index ? ` ${index}-chunked` : ''} password for account:`, account);
182+
return false;
182183
}
183184

184185
async findPassword(service: string): Promise<string | null> {
@@ -190,7 +191,7 @@ export abstract class BaseCredentialsMainService extends Disposable implements I
190191
return null;
191192
}
192193

193-
return keytar.findPassword(service);
194+
return await keytar.findPassword(service);
194195
}
195196

196197
async findCredentials(service: string): Promise<Array<{ account: string; password: string }>> {
@@ -202,7 +203,7 @@ export abstract class BaseCredentialsMainService extends Disposable implements I
202203
return [];
203204
}
204205

205-
return keytar.findCredentials(service);
206+
return await keytar.findCredentials(service);
206207
}
207208

208209
public clear(): Promise<void> {

‎src/vs/workbench/api/browser/mainThreadSecretState.ts

+4
Original file line numberDiff line numberDiff line change
@@ -36,9 +36,11 @@ export class MainThreadSecretState extends Disposable implements MainThreadSecre
3636
}
3737

3838
async $getPassword(extensionId: string, key: string): Promise<string | undefined> {
39+
this.logService.trace(`Getting password for ${extensionId} extension:`, key);
3940
const fullKey = await this.getFullKey(extensionId);
4041
const password = await this.credentialsService.getPassword(fullKey, key);
4142
if (!password) {
43+
this.logService.trace('No password found for:', key);
4244
return undefined;
4345
}
4446

@@ -63,6 +65,7 @@ export class MainThreadSecretState extends Disposable implements MainThreadSecre
6365
try {
6466
const value = JSON.parse(decrypted);
6567
if (value.extensionId === extensionId) {
68+
this.logService.trace('Password found for:', key);
6669
return value.content;
6770
}
6871
} catch (parseError) {
@@ -79,6 +82,7 @@ export class MainThreadSecretState extends Disposable implements MainThreadSecre
7982
}
8083
}
8184

85+
this.logService.trace('No password found for:', key);
8286
return undefined;
8387
}
8488

0 commit comments

Comments
 (0)