プロジェクト

全般

プロフィール

Vote #63225

完了

Add support for two-factor authentication

Admin Redmine さんが約4年前に追加. 約4年前に更新.

ステータス:
Closed
優先度:
通常
担当者:
-
カテゴリ:
Accounts / authentication_7
対象バージョン:
開始日:
2008/05/14
期日:
進捗率:

0%

予定工数:
category_id:
7
version_id:
152
issue_org_id:
1237
author_id:
284
assigned_to_id:
332
comments:
64
status_id:
5
tracker_id:
2
plus1:
4
affected_version:
closed_on:
affected_version_id:
ステータス-->[Closed]

説明

Please add support for a one time password service, such as Yubikey, and add the ability to authenticate against a two-factor authentication system (as in RSA SecurID).


journals

Good idea. Since OpenID (#699) should be implemented too, it could be an idea to build a plugin system for authentication, to help introduction of a new authentification systems.
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------
+1 for dual-factor authentication support

Thanks!
--------------------------------------------------------------------------------
+1 for two factory auth, YubiKey for instance it is open source and well documented
--------------------------------------------------------------------------------
+1 for Two Factory Authentication support
--------------------------------------------------------------------------------
+1 for Two Factory Authentication, maybe using the one-time-password with the Google Authenticator App
--------------------------------------------------------------------------------
This page (http://developers-club.com/posts/168063/) describes a potential method of using a low-level service to achieve this. It looks fairly complicated to set up, but it may help somebody. Would love to hear if it works.
--------------------------------------------------------------------------------
There is a plugin for 2FA including support for SMS, Telegram and authenticator apps:
http://www.redmine.org/plugins/redmine_2fa
--------------------------------------------------------------------------------
Please find attached a proposed 2-factor authentication implementation for Redmine. The core logic for the TOTP scheme is implemented using the "rotp gem":https://rubygems.org/gems/rotp.

The first of the 3 attached patches is an extendable implementation of 2-factor authentication including a default "TOTP":https://en.wikipedia.org/wiki/Time-based_One-time_Password_Algorithm scheme. The second patch adds an option to require, optionally enable or disable 2-factor authentication for all users. The third patch adds support for creating and using backup codes.

The feature is implemented so that new 2-factor authentication schemes other than TOTP can easily be added, for example in a plugin. All scheme-specific localisation strings have a @__#{scheme}__@ identifier, generally this is easier to do with namespaces, i.e. @twofa.#{scheme}.some_string@ instead of @twofa__#{scheme}__some_string@, but the current Redmine tooling only supports top-level localisation keys.

Please furthermore note that the base 2-factor authentication is structured so that it can accommodate computed second factors (TOTP, HOTP, Keyfobs, …) or sent second factors (sending the code via SMS, telegram, …). Those sent second factors can contain a link with the code as query parameter so that the user can click on the link instead of copying the code into the text field in Redmine, which is why all actions that accept a 2-factor authentication code are also available via the @GET@ verb.

We have rolled out this feature to "Planio":https://plan.io a few weeks ago, the customers that use this feature have been happy with it so far.

Last but not least, we have created a plugin implementing a "2-factor email scheme":https://support.plan.io/projects/redmine_twofa_mail/repository to demonstrate how additional schemes can be implemented, and how a scheme sending the one-time-password instead of using a generated one can work with the patches proposed above. Please note that this plugin is only for demonstration purposes and should probably not be used in production, refer to the "Security considerations" section of the README for more details.
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------

--------------------------------------------------------------------------------
Thank Felix and Planio for sharing the patch. I think this is an outstanding enhancement because 2FA is helpful to keep accounts secure and it is a must-have feature for modern cloud-based applications. Many users should welcome this feature.

I tried out the patch. It works very fine.

*2FA is optional by default. Users can choose whether to use 2FA or not:*
!{width: 785px; border: 1px solid #ccc;}2fa-setting@2x.png!

*Users can enable 2FA on "My account" page:*
!{width: 785px; border: 1px solid #ccc;}2fa-my-account@2x.png!

*Enabling 2FA:*
!{width: 785px; border: 1px solid #ccc;}2fa-enabling@2x.png!

*One time password is required as well as login id and password when you sign in:*
!{width: 1024px; border: 1px solid #ccc;}2fa-enter-auth-code@2x.png!
--------------------------------------------------------------------------------
Setting target version to 4.1.0.
--------------------------------------------------------------------------------
What is the release date of 4.1.0. any tentative dates ?
--------------------------------------------------------------------------------
I have translated messages on this patch.
--------------------------------------------------------------------------------
This is an updated Japanese translation. There were 2 untranslated strings in the previous patch.
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------
Felix, could you add tests for this feature?
--------------------------------------------------------------------------------
Thanks Felix!

When applying for the admin account, everything is OK.

But if I applying for normal account user, its error.

********************************************

Started POST "/my/twofa/totp/activate/init" for 127.0.0.1 at 2018-04-10 06:15:23 +0000
Processing by TwofaController#activate_init as HTML
Parameters: {"authenticity_token"=>"Yf7MaaWDeovRvA9IoRYE7hyAZs6aWjgvKB1ELuytNPfzuhN0IhwcIzWQ+df6llld8ksBuclPKCVepCwo0OsAhA==", "scheme"=>"totp"}
Current user: kevin-nguyen (id=6)
Redirected to https://www.enziin.com/my/twofa/totp/activate/confirm
Completed 302 Found in 73ms (ActiveRecord: 12.5ms)
Started GET "/my/twofa/totp/activate/confirm" for 127.0.0.1 at 2018-04-10 06:15:23 +0000
Processing by TwofaController#activate_confirm as HTML
Parameters: {"scheme"=>"totp"}
Current user: kevin-nguyen (id=6)
Rendered twofa/totp/_new.html.erb (422.5ms)
Rendered my/_sidebar.html.erb (14.1ms)
Rendered twofa/activate_confirm.html.erb within layouts/base (459.4ms)
Completed 500 Internal Server Error in 478ms (ActiveRecord: 10.3ms)

ActionView::Template::Error (No route matches {:action=>"destroy", :controller=>"twofa", :scheme=>"totp"}):
4: <%=l(:field_created_on)%>: <%= format_time(@user.created_on) %></p>
5:
6: <% if @user.own_account_deletable? %>
7: <p><%= link_to(l(:button_delete_my_account), {:action => 'destroy'}, :class => 'icon icon-del') %></p>
8: <% end %>
9:
10: <h4><%= l(:label_feeds_access_key) %></h4>
app/views/my/_sidebar.html.erb:7:in `_app_views_my__sidebar_html_erb___731617039739943373_70003011594380'
app/views/twofa/activate_confirm.html.erb:22:in `block in _app_views_twofa_activate_confirm_html_erb__76021097380460310_46976643820260'
app/views/twofa/activate_confirm.html.erb:21:in `_app_views_twofa_activate_confirm_html_erb__76021097380460310_46976643820260'
lib/redmine/sudo_mode.rb:63:in `sudo_mode'

-----------------------------------------------------

I fixed it:

In the "my/_sidebar.html.erb" by adding :controller=>"my"

<% if @user.own_account_deletable? %>
<p><%= link_to(l(:button_delete_my_account), {:action => 'destroy', :controller=>"my"}, :class => 'icon icon-del') %></p>
<% end %>

--------------------------------------------------------------------------------

--------------------------------------------------------------------------------
The @rotp@ gem has changed its API, the attached 0001 patch replaces the original patch and fixes the API changes with rotp.
--------------------------------------------------------------------------------
Enziin System wrote:
> But if I applying for normal account user, its error.

I can see what causes the error. The method to create the link the user can use to delete the user's own account expects the @my/_sidebar.html.erb@ partial to be used only on @/my/account@ or @/my/password@ pages.

I think this should be made more explicit/fixed in Redmine core so that other plugins that may use @my/_sidebar.html.erb@ do not break either. Maeda-san, do you agree? If yes I can open an issue and propose a patch for @my/_sidebar.html.erb@.
--------------------------------------------------------------------------------
I rebased the patch series on top of current master and added an integration test covering the basic activation / totp and backup code functionality. I also fixed the above problem by making the account deletion link more explicit.

--------------------------------------------------------------------------------
Jens Krämer wrote:
> I rebased the patch series on top of current master and added an integration test covering the basic activation / totp and backup code functionality. I also fixed the above problem by making the account deletion link more explicit.

Jens, I think something went wrong because your patches are empty.
--------------------------------------------------------------------------------
I apologize, uploading files on an ipad is still a problem unfortunately :)

--------------------------------------------------------------------------------
the fifth patch of the series actually adds a new feature on top of this so I created a separate issue for it: #31920
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------
Updated first patch that adds a require statement necessary for the patch to work in production.
--------------------------------------------------------------------------------
I have slightly fixed two of the four patches to apply to the latest trunk, r18547. You can add two-factor authentication by applying the following patches.

* attachment:0001-adds-two-factor-authentication-support-r18547.patch (updated)
* attachment:0002-adds-a-setting-to-disable-enable-require-2fa-auth-r18547.patch (updated)
* attachment:0003-backup-codes-for-2fa-auth.patch
* attachment:0004-adds-integration-test-for-totp-two-factor-auth.patch
--------------------------------------------------------------------------------
Two-factor authentication is becoming a must-have feature nowadays. Thanks to Planio, now there is a complete and ready-to-commit patch with test code.

I think it is the time to commit the patch. Setting the target version to 4.1.0.
--------------------------------------------------------------------------------
Go MAEDA wrote:
> Two-factor authentication is becoming a must-have feature nowadays. Thanks to Planio, now there is a complete and ready-to-commit patch with test code.
>
> I thik it is the time to commit the patch. Setting the target version to 4.1.0.

Totally agree with you regarding the need for this feature.

Regarding the patches, I've just tested the entire work and it looks very good to me. All the tests pass on MySQL 5.7 and Postgres 10.9 (https://gitlab.com/redmine-org/redmine/pipelines/85148957).

I've found only some minor things:

*Code related*:
1. The db migration version should be 5.2 in the migration files. Also, the filenames should be updated to the current date before committing.
<pre><code class="diff">
Mariuss-MacBook-Pro:redmine mariusbalteanu$ git diff
diff --git a/db/migrate/20170711134351_add_twofa_scheme_to_user.rb b/db/migrate/20170711134351_add_twofa_scheme_to_user.rb
index ac3f49717..ea0b48fc8 100644
--- a/db/migrate/20170711134351_add_twofa_scheme_to_user.rb
+++ b/db/migrate/20170711134351_add_twofa_scheme_to_user.rb
@@ -1,4 +1,4 @@
-class AddTwofaSchemeToUser < ActiveRecord::Migration[4.2]
+class AddTwofaSchemeToUser < ActiveRecord::Migration[5.2]
def change
add_column :users, :twofa_scheme, :string
end
diff --git a/db/migrate/20170711134352_add_totp_to_user.rb b/db/migrate/20170711134352_add_totp_to_user.rb
index d24bdba86..6842878e3 100644
--- a/db/migrate/20170711134352_add_totp_to_user.rb
+++ b/db/migrate/20170711134352_add_totp_to_user.rb
@@ -1,4 +1,4 @@
-class AddTotpToUser < ActiveRecord::Migration[4.2]
+class AddTotpToUser < ActiveRecord::Migration[5.2]
def change
add_column :users, :twofa_totp_key, :string
add_column :users, :twofa_totp_last_used_at, :integer
</code></pre>

2. The following new Rubocop warnings are introduced:
<pre>
Offenses:

app/controllers/account_controller.rb:255:5: C: Rails/Blank: Use if @user.blank? instead of unless @user.present?.
unless @user.present?
^^^^^^^^^^^^^^^^^^^^^
app/controllers/twofa_backup_codes_controller.rb:1:1: C: Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.
class TwofaBackupCodesController < ApplicationController
^
app/controllers/twofa_controller.rb:1:1: C: Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.
class TwofaController < ApplicationController
^
app/controllers/twofa_controller.rb:78:5: C: Rails/Blank: Use if twofa_scheme.blank? instead of unless twofa_scheme.present?.
unless twofa_scheme.present?
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
app/helpers/twofa_helper.rb:1:1: C: Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.
module TwofaHelper
^
lib/redmine/twofa.rb:1:1: C: Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.
module Redmine
^
lib/redmine/twofa.rb:25:5: W: Lint/UselessAccessModifier: Useless private access modifier.
private
^^^^^^^
lib/redmine/twofa/base.rb:1:1: C: Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.
module Redmine
^
lib/redmine/twofa/base.rb:97:29: C: Style/ParenthesesAroundCondition: Don't use parentheses around the condition of an unless.
return false unless (@user.present? && @user == user_from_code)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
lib/redmine/twofa/totp.rb:1:1: C: Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.
module Redmine
^
test/integration/twofa_test.rb:1:1: C: Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.
require File.expand_path('../../test_helper', __FILE__)
^
test/integration/twofa_test.rb:124:1: C: Layout/EmptyLinesAroundBlockBody: Extra empty line detected at block body end.
</pre>

3. Redmine copywriting "# Redmine - project management software etc" is missing from the files added by these patches.

*Functional:*
4. Users should not be allowed to disable 2FA authentication when the "Two-factor authentication" is set to "required" in administration. Currently, the user can disable it, but at the next login, he is be prompted again to configure 2FA. I propose to not allow users to disable 2FA authentication from the beginning.

--------------------------------------------------------------------------------
Please reassign this to version:"4.1.0" after the above issues are fixed.
--------------------------------------------------------------------------------
Marius, thank you for reviewing the patch series. Here are patches to commit:

* attachment:0001-adds-two-factor-authentication-support-r18593.patch
* attachment:0002-adds-a-setting-to-disable-enable-require-2fa-auth-r18593.patch
* attachment:0003-backup-codes-for-2fa-auth-r18593.patch
* attachment:0004-adds-integration-test-for-totp-two-factor-auth-r18593.patch

Marius BALTEANU wrote:
> 1. The db migration version should be 5.2 in the migration files. Also, the filenames should be updated to the current date before committing.

Applied the patch you posted in #1237#note-31. But the filenames are not changed yet.

> 2. The following new Rubocop warnings are introduced:

All RuboCop warnings are fixed.

> 3. Redmine copywriting "# Redmine - project management software etc" is missing from the files added by these patches.

Done.

> 4. Users should not be allowed to disable 2FA authentication when the "Two-factor authentication" is set to "required" in administration. Currently, the user can disable it, but at the next login, he is be prompted again to configure 2FA. I propose to not allow users to disable 2FA authentication from the beginning.

I think the behavior should be kept. It is necessary to allow users to discard the current 2FA config and set up a new config by themself.

Assume that a user lost their authentication device but they have backup codes, and they want to set up a new authentication device. They can discard the 2FA config for the lost device and configure 2FA for the new device without the help of admins by the following steps:

1. Login with id, password, and a backup code
2. Go to "My account" and Disable 2FA (another backup code is required)
3. Logout
4. Login with id and password. Redmine requires the user to configure 2FA
5. Configure 2FA for the new device

--------------------------------------------------------------------------------
Thanks for updating the patches, I can confirm that all code related issues have been fixed.

One more thing, it seems (at least on my environment) that the patches doesn't apply cleanly anymore:
<pre><code>
mariusbalteanu@Mariuss-MacBook-Pro redmine % curl http://www.redmine.org/attachments/download/24068/0001-adds-two-factor-authentication-support-r18593.patch | patch -p1
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 32734 0 32734 0 0 138k 0 --:--:-- --:--:-- --:--:-- 138k
patching file Gemfile
Hunk #1 succeeded at 21 (offset 3 lines).
patching file app/controllers/account_controller.rb
patching file app/controllers/twofa_controller.rb
patching file app/models/user.rb
patching file app/views/account/twofa_confirm.html.erb
patching file app/views/my/_sidebar.html.erb
patching file app/views/my/account.html.erb
patching file app/views/twofa/activate_confirm.html.erb
patching file app/views/twofa/deactivate_confirm.html.erb
patching file app/views/twofa/totp/_new.html.erb
patching file app/views/users/_form.html.erb
patching file config/locales/de.yml
Hunk #2 FAILED at 1299.
1 out of 2 hunks FAILED -- saving rejects to file config/locales/de.yml.rej
patching file config/locales/en.yml
Hunk #1 succeeded at 1101 (offset 1 line).
Hunk #2 succeeded at 1281 (offset 1 line).
patching file config/routes.rb
patching file db/migrate/20170711134351_add_twofa_scheme_to_user.rb
patching file db/migrate/20170711134352_add_totp_to_user.rb
patching file lib/redmine.rb
patching file lib/redmine/twofa.rb
patching file lib/redmine/twofa/base.rb
patching file lib/redmine/twofa/totp.rb
patching file public/stylesheets/application.css
patching file public/stylesheets/responsive.css
</code></pre>

Can you check, please? I think it is because of r18596.
--------------------------------------------------------------------------------
Marius, thank you for reviewing the patch series. I have updated one of the four patches.

Patches to commit:
* attachment:0001-adds-two-factor-authentication-support-r18640.patch (updated)
* attachment:0002-adds-a-setting-to-disable-enable-require-2fa-auth-r18593.patch
* attachment:0003-backup-codes-for-2fa-auth-r18593.patch
* attachment:0004-adds-integration-test-for-totp-two-factor-auth-r18593.patch
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------

--------------------------------------------------------------------------------

--------------------------------------------------------------------------------

--------------------------------------------------------------------------------
Updated the patch for the current trunk r18859.

Patches to commit:
* attachment:0001-adds-two-factor-authentication-support-r18859.patch (updated)
* attachment:0002-adds-a-setting-to-disable-enable-require-2fa-auth-r18593.patch
* attachment:0003-backup-codes-for-2fa-auth-r18593.patch
* attachment:0004-adds-integration-test-for-totp-two-factor-auth-r18593.patch
--------------------------------------------------------------------------------
Reassigning this huge change to the next major.
--------------------------------------------------------------------------------
Jean-Philippe Lang wrote:
> Reassigning this huge change to the next major.

I think the next major release is 4.2.0.
--------------------------------------------------------------------------------
I have emailed Jean-Philippe Lang and got his approval to commit this patch for Redmine 4.2.0.

Here are the final patches to be committed. I will commit the patches in a few days to a week.

* attachment:0001-Adds-two-factor-authentication-support-1237.patch
* attachment:0002-Adds-a-setting-to-disable-enable-require-2fa-auth-12.patch
* attachment:0003-Backup-codes-for-2fa-auth-1237.patch
* attachment:0004-Integration-test-for-2fa-auth-1237.patch
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------

--------------------------------------------------------------------------------

--------------------------------------------------------------------------------

--------------------------------------------------------------------------------
Go MAEDA wrote:
> I have emailed Jean-Philippe Lang and got his approval to commit this patch for Redmine 4.2.0.

Great news!
--------------------------------------------------------------------------------
Go MAEDA wrote:
> I have emailed Jean-Philippe Lang and got his approval to commit this patch for Redmine 4.2.0.

That's so cool! Thank you for your efforts.

--------------------------------------------------------------------------------
Committed the patches from Planio. Thank you for your great contribution.
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------

--------------------------------------------------------------------------------

--------------------------------------------------------------------------------

--------------------------------------------------------------------------------

--------------------------------------------------------------------------------

--------------------------------------------------------------------------------

--------------------------------------------------------------------------------

--------------------------------------------------------------------------------

--------------------------------------------------------------------------------

--------------------------------------------------------------------------------

--------------------------------------------------------------------------------

--------------------------------------------------------------------------------

--------------------------------------------------------------------------------

--------------------------------------------------------------------------------


related_issues

relates,Closed,699,OpenID login
relates,Closed,33925,ArgumentError in lib/redmine/twofa/base.rb after updating a source file in development mode
relates,Closed,33929,Encrypt / decrypt TOTP secret keys with `rake db:encrypt` / `rake db:decrypt`
relates,Closed,33932,Can't view "My account" page when 2fa is setting to optional
relates,Closed,34233,otpauth uri should include a login id, not an email address
relates,Closed,34234,Use Setting.host_name instead of Setting.app_title as TOTP issuer to avoid name collision with other instances or apps
relates,Closed,34241, Include twofa_scheme (two-factor scheme) column when exporting users to CSV
relates,Closed,34495,Don't show "Two-factor authentication" on new user form
relates,Closed,34618,Cannot sign in when both enabling two-factor authentication and changing password are required
relates,New,34070,Allow setting a grace period when forcing 2FA
relates,Closed,35372,Better presentation for 2FA recovery codes
relates,Closed,35001,Disable API authentication with username and password when two-factor authentication is enabled for the user
relates,Closed,35086,Please consider changing the way how 2FA is set up
relates,Closed,35439,Option to require 2FA only for users with administration rights
relates,Closed,35934,Show 2FA status in users list from administration with option to filter
blocks,Closed,31920,Require 2FA only for certain user groups

Admin Redmine さんが約4年前に更新

  • カテゴリAccounts / authentication_7 にセット
  • 対象バージョン4.2.0_152 にセット

他の形式にエクスポート: Atom PDF

いいね!0
いいね!0