Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Remove keytar and gnome-keyring deps #10514

Merged
merged 1 commit into from
Aug 17, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -37,4 +37,3 @@ addons:
packages:
- xvfb
- g++-4.8
- libgnome-keyring-dev
Copy link
Member

Choose a reason for hiding this comment

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

gnome-keyring is needed at runtime by muon, not sure if removing this will cause issues.
see: #10448

Copy link
Member

Choose a reason for hiding this comment

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

gnome-keyring is deprecated by libsecret

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

And I think this is the deps for keytar use not for muon which will be precompiled before browser-laptop can use it.

4 changes: 2 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -59,13 +59,13 @@ For other platforms (macOS, Linux) You'll need certain packages installed before
#### On Debian / Ubuntu /Mint

````
apt-get install libgnome-keyring-dev build-essential rpm ninja-build
apt-get install build-essential rpm ninja-build
````

#### On Fedora

````
dnf install libgnome-keyring-devel rpm-build
dnf install rpm-build
dnf group install "Development Tools" "C Development Tools and Libraries"
````

Expand Down
116 changes: 9 additions & 107 deletions app/browser/reducers/passwordManagerReducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,83 +4,18 @@

'use strict'

const keytar = require('keytar')
const appConstants = require('../../../js/constants/appConstants')
const appActions = require('../../../js/actions/appActions')
const CryptoUtil = require('../../../js/lib/cryptoUtil')
const locale = require('../../locale')
const messages = require('../../../js/constants/messages')
const {getWebContents} = require('../webContentsCache')
const {makeImmutable} = require('../../common/state/immutableUtil')
const Immutable = require('immutable')
const {ipcMain} = require('electron')
const autofill = require('../../autofill')

const unsafeTestMasterKey = 'c66af15fc6555ebecf7cee3a5b82c108fd3cb4b587ab0b299d28e39c79ecc708'

// Don't show the keytar prompt more than once per 24 hours
let throttleKeytar = false

// Map of password notification bar messages to their callbacks
const passwordCallbacks = {}

let masterKey

/**
* Gets the master key for encrypting login credentials from the OS keyring.
*/
const getMasterKey = () => {
if (throttleKeytar) {
return null
}

if (process.env.NODE_ENV === 'test') {
// workaround for https://travis-ci.org/brave/browser-laptop/builds/132700770
return (new Buffer(unsafeTestMasterKey, 'hex')).toString('binary')
}

const appName = 'Brave'
// Previously the master key was binary encoded, which caused compatibility
// issues with various keyrings. In 0.8.3, switch to hex encoding for storage.
const oldAccountName = 'login master key'
const accountName = 'login master key v2'
let oldMasterKey = keytar.getPassword(appName, oldAccountName)
let masterKey = keytar.getPassword(appName, accountName)

let success = false

if (masterKey === null) {
if (typeof oldMasterKey === 'string') {
// The user made a v1 (binary) key. Try converting it to hex if it
// appears to be 32 bytes.
let oldBuffer = new Buffer(oldMasterKey, 'binary')
if (oldBuffer.length === 32) {
success = keytar.addPassword(appName, accountName, oldBuffer.toString('hex'))
}
}

// Either the user denied access or no master key has ever been created.
// We can't tell the difference so try making a new master key.
success = success || keytar.addPassword(appName, accountName, CryptoUtil.getRandomBytes(32).toString('hex'))

if (success) {
// A key should have been created
masterKey = keytar.getPassword(appName, accountName)
}
}

if (typeof masterKey === 'string') {
// Convert from hex to binary
return (new Buffer(masterKey, 'hex')).toString('binary')
} else {
throttleKeytar = true
setTimeout(() => {
throttleKeytar = false
}, 1000 * 60 * 60 * 24)
return null
}
}

const init = () => {
ipcMain.on(messages.NOTIFICATION_RESPONSE, (e, message, buttonIndex, persist) => {
if (passwordCallbacks[message]) {
Expand Down Expand Up @@ -190,53 +125,20 @@ const clearPasswords = () => {
autofill.clearLogins()
}

const migrate = (state) => {
const passwords = state.get('passwords')
if (passwords.size) {
masterKey = masterKey || getMasterKey()
if (!masterKey) {
console.log('Could not access master password; aborting')
return
}
passwords.forEach((password) => {
let decrypted = CryptoUtil.decryptVerify(password.get('encryptedPassword'),
password.get('authTag'),
masterKey,
password.get('iv'))
if (decrypted) {
let form = {}
form['origin'] = password.get('origin')
form['signon_realm'] = password.get('origin') + '/'
form['action'] = password.get('action')
form['username'] = password.get('username')
form['password'] = decrypted
autofill.addLogin(form)
}
})
state = state.set('legacyPasswords', state.get('passwords'))
state = state.set('passwords', new Immutable.List())
}
const allSiteSettings = state.get('siteSettings')
const blackedList = allSiteSettings.filter((setting) => setting.get('savePasswords') === false)
if (blackedList.size) {
blackedList.forEach((entry, index) => {
let form = {}
form['origin'] = index
form['signon_realm'] = index + '/'
form['blacklisted_by_user'] = true
autofill.addLogin(form)
appActions.deletePasswordSite(index)
})
}
return state
}

const passwordManagerReducer = (state, action, immutableAction) => {
action = immutableAction || makeImmutable(action)
switch (action.get('actionType')) {
case appConstants.APP_SET_STATE:
init()
state = migrate(state)
Copy link
Member

Choose a reason for hiding this comment

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

I think we should also check if legacyPasswords exists or not. If so, we should delete it to prevent our session data from getting bloated.

Copy link
Member Author

Choose a reason for hiding this comment

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

done!

// Log a warning if they are updating from <0.15.300 to 0.21 or higher
if (state.getIn(['passwords', 0])) {
console.log('Warning: unable to migrate old passwords.')
}
// legacyPasswords was added in 0.15.300 to backup old passwords in case
// the migration failed
if (state.get('legacyPasswords')) {
state = state.delete('legacyPasswords')
}
break
case appConstants.APP_SAVE_PASSWORD:
savePassword(action.get('username'), action.get('origin'), action.get('tabId'))
Expand Down
18 changes: 0 additions & 18 deletions docs/state.md
Original file line number Diff line number Diff line change
Expand Up @@ -192,24 +192,6 @@ AppStore
noScript: {
enabled: boolean // enable noscript
},
passwords: [{
// not being used anymore
action: string, // URL of the form action
authTag: string, // AES-GCM authentication data, binary-encoded
encryptedPassword: string, // encrypted by master password, binary-encoded
iv: string, // AES-GCM initialization vector, binary-encoded
origin: string, // origin of the form
username: string
}],
legacyPasswords: [{
// backup of passwords migration
action: string, // URL of the form action
authTag: string, // AES-GCM authentication data, binary-encoded
encryptedPassword: string, // encrypted by master password, binary-encoded
iv: string, // AES-GCM initialization vector, binary-encoded
origin: string, // origin of the form
username: string
}],
pinnedSites: {
[siteKey]: {
location: string,
Expand Down
112 changes: 0 additions & 112 deletions js/lib/cryptoUtil.js

This file was deleted.

1 change: 0 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,6 @@
"immutable": "^3.7.5",
"immutablediff": "^0.4.2",
"immutablepatch": "brave/immutable-js-patch",
"keytar": "^3.0.0",
"l20n": "^3.5.1",
"ledger-balance": "^0.9.1",
"ledger-client": "^0.9.18",
Expand Down
4 changes: 1 addition & 3 deletions res/linuxPackaging.json
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,6 @@
"libxtst6",
"libxss1",
"python",
"xdg-utils",
"gir1.2-gnomekeyring-1.0",
"libgnome-keyring0"
"xdg-utils"
]
}
1 change: 0 additions & 1 deletion test/unit/app/sessionStoreShutdownTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,6 @@ describe('sessionStoreShutdown unit tests', function () {
mockery.registerMock('electron', fakeElectron)
mockery.registerMock('ad-block', fakeAdBlock)
mockery.registerMock('leveldown', {})
mockery.registerMock('keytar', {})
sessionStore = require('../../../app/sessionStore')
sessionStoreShutdown = require('../../../app/sessionStoreShutdown')
appActions = require('../../../js/actions/appActions')
Expand Down
Loading