Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(555)

Issue 2051003: Initial patch from Will. (Closed)

Created:
10 years, 7 months ago by fes
Modified:
9 years, 7 months ago
CC:
chromium-os-reviews_chromium.org, Chris Masone, gauravsh, Will Drewry, rginda
Base URL:
ssh://git@chromiumos-git/chromiumos
Visibility:
Public.

Description

Initial patch from Will.

Patch Set 1 #

Patch Set 2 : Use echo -n to remove newline so that it is consistent with original/authenticator.cc #

Patch Set 3 : Add GetSystemSalt call to cryptohome, backend pam_offline to cryptohome. #

Patch Set 4 : Add string constant for GetSystemSalt. #

Patch Set 5 : Concentrate password->passkey into UsernamePasshash. #

Total comments: 3

Patch Set 6 : Make ecryptfs use the key as a key instead of a passphrase. #

Patch Set 7 : Remove tick counting code now that the delay is fixed. #

Patch Set 8 : Have cryptohome re-create the directory on key unwrap error. Copy /etc/skel. #

Patch Set 9 : Add missing file and have init scrips use cryptohome instead of mount.cryptohome. #

Patch Set 10 : Re-sync. #

Patch Set 11 : Add removal of dm-crypt image file. #

Patch Set 12 : Update with Will's quick-fix list. #

Patch Set 13 : Missed a few review points--updating with those. #

Patch Set 14 : Move the terminate calls around to better support non-lazy umount. #

Patch Set 15 : Re-allow lazy unmount. #

Patch Set 16 : Should be final lazy unmount change. #

Patch Set 17 : Style--missing space after 'if'. #

Total comments: 3

Patch Set 18 : Address style nits. #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+2746 lines, -1572 lines) Patch
M src/common/chromeos/dbus/service_constants.h View 3 4 5 6 7 8 9 10 1 chunk +3 lines, -0 lines 0 comments Download
M src/common/chromeos/dbus/service_constants.cc View 3 4 5 6 7 8 9 10 1 chunk +3 lines, -0 lines 1 comment Download
M src/platform/cryptohome/SConstruct View 5 6 7 8 9 10 11 12 3 chunks +14 lines, -10 lines 0 comments Download
M src/platform/cryptohome/authenticator.h View 3 4 1 chunk +0 lines, -102 lines 0 comments Download
D src/platform/cryptohome/authenticator.cc View 1 chunk +0 lines, -254 lines 0 comments Download
D src/platform/cryptohome/authenticator_unittest.cc View 1 chunk +0 lines, -114 lines 0 comments Download
M src/platform/cryptohome/bin/mount View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +0 lines, -103 lines 0 comments Download
M src/platform/cryptohome/bin/umount View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +0 lines, -80 lines 0 comments Download
D src/platform/cryptohome/check_cryptohome_data.sh View 1 chunk +0 lines, -106 lines 0 comments Download
M src/platform/cryptohome/credentials.h View 3 chunks +7 lines, -14 lines 0 comments Download
A src/platform/cryptohome/cryptohome.cc View 5 6 7 8 9 10 11 12 1 chunk +337 lines, -0 lines 0 comments Download
M src/platform/cryptohome/cryptohome.xml View 3 4 5 6 7 8 9 10 1 chunk +13 lines, -0 lines 0 comments Download
A src/platform/cryptohome/cryptohome_common.h View 6 7 8 9 10 11 1 chunk +44 lines, -0 lines 1 comment Download
M src/platform/cryptohome/cryptohomed.cc View 2 chunks +0 lines, -21 lines 0 comments Download
A src/platform/cryptohome/entropy_source.h View 1 chunk +22 lines, -0 lines 0 comments Download
M src/platform/cryptohome/etc/Cryptohome.conf View 3 4 5 6 7 8 9 10 1 chunk +9 lines, -0 lines 0 comments Download
D src/platform/cryptohome/example_client.cc View 1 chunk +0 lines, -83 lines 0 comments Download
M src/platform/cryptohome/init_cryptohome_data.sh View 5 6 7 2 chunks +42 lines, -8 lines 0 comments Download
M src/platform/cryptohome/interface.h View 3 4 5 6 7 8 9 10 11 2 chunks +15 lines, -2 lines 0 comments Download
M src/platform/cryptohome/interface.cc View 3 4 5 6 7 8 9 10 11 2 chunks +23 lines, -4 lines 0 comments Download
D src/platform/cryptohome/mock_authenticator.h View 1 chunk +0 lines, -24 lines 0 comments Download
A src/platform/cryptohome/mock_mount.h View 1 chunk +24 lines, -0 lines 0 comments Download
A src/platform/cryptohome/mount.h View 5 6 7 8 9 10 11 12 13 14 1 chunk +387 lines, -0 lines 0 comments Download
A src/platform/cryptohome/mount.cc View 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +998 lines, -0 lines 2 comments Download
A src/platform/cryptohome/mount_unittest.cc View 5 6 7 1 chunk +178 lines, -0 lines 0 comments Download
A src/platform/cryptohome/secure_blob.h View 5 1 chunk +30 lines, -0 lines 0 comments Download
A src/platform/cryptohome/secure_blob.cc View 5 1 chunk +40 lines, -0 lines 0 comments Download
M src/platform/cryptohome/service.h View 3 4 5 6 7 8 9 10 4 chunks +14 lines, -21 lines 0 comments Download
M src/platform/cryptohome/service.cc View 3 4 5 6 7 8 9 10 11 12 13 4 chunks +80 lines, -47 lines 0 comments Download
M src/platform/cryptohome/service_unittest.cc View 2 chunks +4 lines, -38 lines 0 comments Download
D src/platform/cryptohome/username_passhash.h View 1 chunk +0 lines, -51 lines 0 comments Download
D src/platform/cryptohome/username_passhash.cc View 1 chunk +0 lines, -55 lines 0 comments Download
A src/platform/cryptohome/username_passkey.h View 1 chunk +60 lines, -0 lines 0 comments Download
A src/platform/cryptohome/username_passkey.cc View 5 6 7 8 9 10 11 1 chunk +138 lines, -0 lines 1 comment Download
A + src/platform/cryptohome/username_passkey_unittest.cc View 3 chunks +20 lines, -21 lines 0 comments Download
A src/platform/cryptohome/vault_keyset.h View 1 chunk +54 lines, -0 lines 0 comments Download
A src/platform/cryptohome/vault_keyset.cc View 6 7 8 9 10 11 1 chunk +140 lines, -0 lines 0 comments Download
M src/platform/init/chromeos_shutdown View 1 chunk +1 line, -1 line 0 comments Download
M src/platform/init/ui.conf View 1 chunk +2 lines, -6 lines 0 comments Download
M src/platform/pam_offline/SConstruct View 2 chunks +6 lines, -5 lines 0 comments Download
D src/platform/pam_offline/authenticator.h View 1 chunk +0 lines, -98 lines 0 comments Download
D src/platform/pam_offline/authenticator.cc View 1 chunk +0 lines, -210 lines 0 comments Download
D src/platform/pam_offline/authenticator_unittest.cc View 1 chunk +0 lines, -73 lines 0 comments Download
M src/platform/pam_offline/pam_offline.cc View 3 4 5 6 7 8 9 10 11 3 chunks +40 lines, -18 lines 0 comments Download
D src/platform/pam_offline/test_image_dir/05c059b850095e85443627071e9551487401148f/master.0 View 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +0 lines, -1 line 0 comments Download
D src/platform/pam_offline/test_image_dir/05c059b850095e85443627071e9551487401148f/master.1 View 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +0 lines, -1 line 0 comments Download
D src/platform/pam_offline/test_image_dir/05c059b850095e85443627071e9551487401148f/master.2 View 0 chunks +-1 lines, --1 lines 0 comments Download
D src/platform/pam_offline/test_image_dir/05c059b850095e85443627071e9551487401148f/master.0.salt View 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +0 lines, -1 line 0 comments Download
D src/platform/pam_offline/test_image_dir/05c059b850095e85443627071e9551487401148f/master.1.salt View 0 chunks +-1 lines, --1 lines 0 comments Download
D src/platform/pam_offline/test_image_dir/05c059b850095e85443627071e9551487401148f/master.2.salt View 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +0 lines, -1 line 0 comments Download
D src/platform/pam_offline/test_image_dir/salt View 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +0 lines, -1 line 0 comments Download

Messages

Total messages: 6 (0 generated)
fes
10 years, 7 months ago (2010-05-07 19:32:28 UTC) #1
fes
10 years, 7 months ago (2010-05-12 20:50:15 UTC) #2
fes
10 years, 7 months ago (2010-05-12 20:54:24 UTC) #3
Will Drewry
I reviewed the diff on the plane and attached the vim quickfix err notes from ...
10 years, 7 months ago (2010-05-26 20:13:02 UTC) #4
Will Drewry
LGTM with style nits Awesome. http://codereview.chromium.org/2051003/diff/44001/45023 File src/platform/cryptohome/mount.cc (right): http://codereview.chromium.org/2051003/diff/44001/45023#newcode21 src/platform/cryptohome/mount.cc:21: #include <dirent.h> style nit: ...
10 years, 7 months ago (2010-05-27 00:09:51 UTC) #5
mschilder
10 years, 7 months ago (2010-05-27 04:20:18 UTC) #6
http://codereview.chromium.org/2051003/diff/47001/39052
File src/common/chromeos/dbus/service_constants.cc (right):

http://codereview.chromium.org/2051003/diff/47001/39052#newcode17
src/common/chromeos/dbus/service_constants.cc:17: const char
*kCryptohomeMigrateKey = "MigrateKey";
Should these be const char * const? Now you allocate a malleable .data ptr for
each. The compiler might optimize const * const away entirely, e.g. equiv to
const char foo[] = ""

http://codereview.chromium.org/2051003/diff/47001/39064
File src/platform/cryptohome/cryptohome_common.h (right):

http://codereview.chromium.org/2051003/diff/47001/39064#newcode22
src/platform/cryptohome/cryptohome_common.h:22: #define CRYPTOHOME_MIN(X, Y)
(((X) < (Y)) ? (X) : (Y))
These are scary of course, double eval of arguments;
CRYPTOHOME_MIN(x++, y--) type stuff.

gcc-ism like a >? b or typeof can help?

http://codereview.chromium.org/2051003/diff/47001/39074
File src/platform/cryptohome/mount.cc (right):

http://codereview.chromium.org/2051003/diff/47001/39074#newcode84
src/platform/cryptohome/mount.cc:84: struct passwd* user_info =
getpwnam(default_username_.c_str());
getpwnam_r() instead?

http://codereview.chromium.org/2051003/diff/47001/39074#newcode899
src/platform/cryptohome/mount.cc:899: while (struct dirent* fd_dirent =
readdir(fd_dir)) {
readdir_r()?

http://codereview.chromium.org/2051003/diff/47001/39084
File src/platform/cryptohome/username_passkey.cc (right):

http://codereview.chromium.org/2051003/diff/47001/39084#newcode80
src/platform/cryptohome/username_passkey.cc:80:
reinterpret_cast<char*>(&password_hash[0]),
Add a void* data() method to SecureBlob? This casting is not very future proof
in case the implementation details of SecureBlob change. From this context it's
not clear what SecureBlob is. The [0] suggests it's a typedef, not a class?

Avoid reinterpret_cast like the plague.

Powered by Google App Engine
This is Rietveld 408576698