{"action":"created","issue":{"url":"https://api.github.com/repos/ansible/ansible/issues/15060","repository_url":"https://api.github.com/repos/ansible/ansible","labels_url":"https://api.github.com/repos/ansible/ansible/issues/15060/labels{/name}","comments_url":"https://api.github.com/repos/ansible/ansible/issues/15060/comments","events_url":"https://api.github.com/repos/ansible/ansible/issues/15060/events","html_url":"https://github.com/ansible/ansible/pull/15060","id":142263682,"number":15060,"title":"Vault improvements: base64 encoding, no AES-CTR padding, code cleanups","user":{"login":"amenonsen","id":23628,"avatar_url":"https://avatars0.githubusercontent.com/u/23628?v=3","gravatar_id":"","url":"https://api.github.com/users/amenonsen","html_url":"https://github.com/amenonsen","followers_url":"https://api.github.com/users/amenonsen/followers","following_url":"https://api.github.com/users/amenonsen/following{/other_user}","gists_url":"https://api.github.com/users/amenonsen/gists{/gist_id}","starred_url":"https://api.github.com/users/amenonsen/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/amenonsen/subscriptions","organizations_url":"https://api.github.com/users/amenonsen/orgs","repos_url":"https://api.github.com/users/amenonsen/repos","events_url":"https://api.github.com/users/amenonsen/events{/privacy}","received_events_url":"https://api.github.com/users/amenonsen/received_events","type":"User","site_admin":false},"labels":[{"id":454885231,"url":"https://api.github.com/repos/ansible/ansible/labels/affects_2.3","name":"affects_2.3","color":"5319e7","default":false},{"id":48276098,"url":"https://api.github.com/repos/ansible/ansible/labels/bugfix_pull_request","name":"bugfix_pull_request","color":"ded270","default":false},{"id":511882096,"url":"https://api.github.com/repos/ansible/ansible/labels/c:parsing/vault/","name":"c:parsing/vault/","color":"b60205","default":false},{"id":48457821,"url":"https://api.github.com/repos/ansible/ansible/labels/feature_pull_request","name":"feature_pull_request","color":"006b75","default":false},{"id":231201931,"url":"https://api.github.com/repos/ansible/ansible/labels/needs_rebase","name":"needs_rebase","color":"eb6420","default":false},{"id":61436281,"url":"https://api.github.com/repos/ansible/ansible/labels/needs_revision","name":"needs_revision","color":"eb6450","default":false},{"id":581543918,"url":"https://api.github.com/repos/ansible/ansible/labels/stale_ci","name":"stale_ci","color":"fef2c0","default":false},{"id":461464968,"url":"https://api.github.com/repos/ansible/ansible/labels/test_pull_requests","name":"test_pull_requests","color":"0e8a16","default":false}],"state":"open","locked":false,"assignee":{"login":"alikins","id":15162,"avatar_url":"https://avatars3.githubusercontent.com/u/15162?v=3","gravatar_id":"","url":"https://api.github.com/users/alikins","html_url":"https://github.com/alikins","followers_url":"https://api.github.com/users/alikins/followers","following_url":"https://api.github.com/users/alikins/following{/other_user}","gists_url":"https://api.github.com/users/alikins/gists{/gist_id}","starred_url":"https://api.github.com/users/alikins/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/alikins/subscriptions","organizations_url":"https://api.github.com/users/alikins/orgs","repos_url":"https://api.github.com/users/alikins/repos","events_url":"https://api.github.com/users/alikins/events{/privacy}","received_events_url":"https://api.github.com/users/alikins/received_events","type":"User","site_admin":false},"assignees":[{"login":"alikins","id":15162,"avatar_url":"https://avatars3.githubusercontent.com/u/15162?v=3","gravatar_id":"","url":"https://api.github.com/users/alikins","html_url":"https://github.com/alikins","followers_url":"https://api.github.com/users/alikins/followers","following_url":"https://api.github.com/users/alikins/following{/other_user}","gists_url":"https://api.github.com/users/alikins/gists{/gist_id}","starred_url":"https://api.github.com/users/alikins/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/alikins/subscriptions","organizations_url":"https://api.github.com/users/alikins/orgs","repos_url":"https://api.github.com/users/alikins/repos","events_url":"https://api.github.com/users/alikins/events{/privacy}","received_events_url":"https://api.github.com/users/alikins/received_events","type":"User","site_admin":false}],"milestone":null,"comments":10,"created_at":"2016-03-21T05:38:07Z","updated_at":"2017-05-24T00:51:29Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/ansible/ansible/pulls/15060","html_url":"https://github.com/ansible/ansible/pull/15060","diff_url":"https://github.com/ansible/ansible/pull/15060.diff","patch_url":"https://github.com/ansible/ansible/pull/15060.patch"},"body":"##### Issue Type:\n- Feature Pull Request\n##### Summary:\n\nIntroduce an improved AES256 cipher version\n\nNow we don't add spurious padding for AES-CTR (which is used as a stream cipher, and doesn't need padding) and we use base64 to encode the output message instead of blowing up its size by hexlify()ing everything twice. We also don't unnecessarily use PBKDF2 to generate the initial value for the encryption counter.\n\nWe always use the new format for output, but maintain read-compatibility with a few lines of 1.1-specific decoding in decrypt(). (Note that we do not force v1.1 files to be rewritten by ansible-vault edit unless their\ncontents have actually changed, in which case the new cipher version is used automatically).\n\nCloses #10634 from graingert (which removed the padding, but added yet another cipher class for backwards compatibility)\n\nFixes #12121 from mgedmin (which reported the double-hexlify() problem)\n"},"comment":{"url":"https://api.github.com/repos/ansible/ansible/issues/comments/303582982","html_url":"https://github.com/ansible/ansible/pull/15060#issuecomment-303582982","issue_url":"https://api.github.com/repos/ansible/ansible/issues/15060","id":303582982,"user":{"login":"Lekensteyn","id":164530,"avatar_url":"https://avatars3.githubusercontent.com/u/164530?v=3","gravatar_id":"","url":"https://api.github.com/users/Lekensteyn","html_url":"https://github.com/Lekensteyn","followers_url":"https://api.github.com/users/Lekensteyn/followers","following_url":"https://api.github.com/users/Lekensteyn/following{/other_user}","gists_url":"https://api.github.com/users/Lekensteyn/gists{/gist_id}","starred_url":"https://api.github.com/users/Lekensteyn/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Lekensteyn/subscriptions","organizations_url":"https://api.github.com/users/Lekensteyn/orgs","repos_url":"https://api.github.com/users/Lekensteyn/repos","events_url":"https://api.github.com/users/Lekensteyn/events{/privacy}","received_events_url":"https://api.github.com/users/Lekensteyn/received_events","type":"User","site_admin":false},"created_at":"2017-05-24T00:51:29Z","updated_at":"2017-05-24T00:51:29Z","body":"Hey, the idea of this change (use base64 instead of double hex; remove unnecessary padding) looks good to me. (Maybe the cleanups could be split a bit in separate commits, I skipped over them because it contains a lot of unrelated changes which made reviewing harder.)\r\n\r\n(I have done a small audit of the existing Ansible Vault code, and while the old \"AES\" variant is broken due to a weak KDF, the \"AES256\" seems \"okay\" other than wasting space (double hex) and unnecessary complexity (incl. padding.)\r\n\r\nSome other comments:\r\n\r\n- For forwards compat, shouldn't \"b_version\" be checked (and error out or warn)? A quick look in the `parsing/vault/__init__.py`  file did not show a user of the `b_version` field. Older clients will presumably crash on data generated by new programs because `unhexlify(b_vaulttext)` chokes on base64 data. Throwin an error would clearly signal that the user that the program is too old to understand it. Warning (or silently ignoring as is done now) would allow for decrypting containers if the format has not drastically been changed.\r\n- Ideally the PBKDF2 iteration count is also stored in the file such that it can be increased in the future if needed.\r\n- (Meta-comment: I wonder how much prior research has been done on encrypted container formats, some of these issues were avoidable...)"}}
{"actor":{"display_login":"Lekensteyn"}}