gitea
/
Koha-clms
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Bug 37893: (QA follow-up): Remove plaintext password storage from SIP2 

This follow-up removes the security vulnerability of storing plaintext
passwords in the sip_accounts table. The SIP2 system now relies entirely
on Koha's secure password authentication.

Changes:
- Remove plaintext password comparison from login_core() and telnet_transport()
- Remove login_password column from sip_accounts table
- Update database schema files and atomic update
- Remove password field from API definitions and Vue form
- Update all tests to remove login_password references

Authentication now works in two stages:
1. Access control: Check if userid exists in sip_accounts (allowlist)
2. Authentication: Validate password against hash in borrowers table

This maintains SIP access control while eliminating plaintext password
storage. Staff users authenticate with their Koha password (securely hashed).

Test plan:
1. Run updated tests to verify they pass
2. Verify SIP2 authentication works with staff user credentials
3. Confirm no plaintext passwords are stored

Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This commit is contained in:
Martin Renvoize 2025-10-24 15:49:21 +01:00 committed by Lucas Gass
parent dfd9e08b6b
commit c1a97af67c
No known key found for this signature in database
GPG Key ID: 60B447398B0D36CA
10 changed files with 64 additions and 90 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
C4/SIP/SIPServer.pm
View File

@ -423,9 +423,8 @@ sub telnet_transport {
$pwd = get_clean_string($pwd);
siplog( "LOG_DEBUG", "telnet_transport 2: uid length %s, pwd length %s", length($uid), length($pwd) );
if ( exists( $config->{accounts}->{$uid} )
&& ( $pwd eq $config->{accounts}->{$uid}->{password} ) )
{
# Check if user is authorized for SIP access, then authenticate via login_core
if ( exists( $config->{accounts}->{$uid} ) ) {
$account = $config->{accounts}->{$uid};
if ( C4::SIP::Sip::MsgType::login_core( $self, $uid, $pwd ) ) {
last;

102
C4/SIP/Sip/MsgType.pm
View File

@ -927,59 +927,59 @@ sub login_core {
my $uid = shift;
my $pwd = shift;
my $status = 1; # Assume it all works
# Check if this userid is authorized for SIP access
if ( !exists( $server->{config}->{accounts}->{$uid} ) ) {
siplog( "LOG_WARNING", "MsgType::login_core: Unknown login '$uid'" );
$status = 0;
} elsif ( $server->{config}->{accounts}->{$uid}->{password} ne $pwd ) {
siplog( "LOG_WARNING", "MsgType::login_core: Invalid password for login '$uid'" );
$status = 0;
} else {
# Store the active account someplace handy for everybody else to find.
$server->{account} = $server->{config}->{accounts}->{$uid};
my $inst = $server->{account}->{institution};
$server->{institution} = $server->{config}->{institutions}->{$inst};
$server->{policy} = $server->{institution}->{policy};
$server->{sip_username} = $uid;
$server->{sip_password} = $pwd;
my $auth_status = api_auth( $uid, $pwd, $inst );
if ( !$auth_status or $auth_status !~ /^ok$/i ) {
siplog(
"LOG_WARNING", "api_auth failed for SIP terminal '%s' of '%s': %s", $uid, $inst,
( $auth_status || 'unknown' )
);
$status = 0;
} else {
siplog( "LOG_INFO", "Successful login/auth for '%s' of '%s'", $server->{account}->{id}, $inst );
#
# initialize connection to ILS
#
my $module = $server->{config}->{institutions}->{$inst}->{implementation};
siplog( "LOG_DEBUG", 'login_core: ' . Dumper($module) );
# Suspect this is always ILS but so we don't break any eccentic install (for now)
if ( $module eq 'ILS' ) {
$module = 'C4::SIP::ILS';
}
$module->use;
if ($@) {
siplog(
"LOG_ERR", "%s: Loading ILS implementation '%s' for institution '%s' failed",
$server->{service}, $module, $inst
);
die("Failed to load ILS implementation '$module' for $inst");
}
# like ILS->new(), I think.
$server->{ils} = $module->new( $server->{institution}, $server->{account} );
if ( !$server->{ils} ) {
siplog( "LOG_ERR", "%s: ILS connection to '%s' failed", $server->{service}, $inst );
die("Unable to connect to ILS '$inst'");
}
}
siplog( "LOG_WARNING", "MsgType::login_core: SIP access not authorized for user '$uid'" );
return 0;
}
# Store the active account configuration
$server->{account} = $server->{config}->{accounts}->{$uid};
my $inst = $server->{account}->{institution};
$server->{institution} = $server->{config}->{institutions}->{$inst};
$server->{policy} = $server->{institution}->{policy};
$server->{sip_username} = $uid;
$server->{sip_password} = $pwd;
# Authenticate using Koha's internal authentication (checks hashed password in borrowers table)
my $auth_status = api_auth( $uid, $pwd, $inst );
if ( !$auth_status or $auth_status !~ /^ok$/i ) {
siplog(
"LOG_WARNING", "Authentication failed for SIP terminal '%s' of '%s': %s",
$uid, $inst, ( $auth_status || 'unknown' )
);
return 0;
}
siplog( "LOG_INFO", "Successful login/auth for '%s' of '%s'", $server->{account}->{id}, $inst );
#
# initialize connection to ILS
#
my $module = $server->{config}->{institutions}->{$inst}->{implementation};
siplog( "LOG_DEBUG", 'login_core: ' . Dumper($module) );
# Suspect this is always ILS but so we don't break any eccentic install (for now)
if ( $module eq 'ILS' ) {
$module = 'C4::SIP::ILS';
}
$module->use;
if ($@) {
siplog(
"LOG_ERR", "%s: Loading ILS implementation '%s' for institution '%s' failed",
$server->{service}, $module, $inst
);
die("Failed to load ILS implementation '$module' for $inst");
}
# like ILS->new(), I think.
$server->{ils} = $module->new( $server->{institution}, $server->{account} );
if ( !$server->{ils} ) {
siplog( "LOG_ERR", "%s: ILS connection to '%s' failed", $server->{service}, $inst );
die("Unable to connect to ILS '$inst'");
}
return $status;
}

1
Koha/SIP2/Account.pm
View File

@ -69,7 +69,6 @@ sub get_for_config {
# Map database columns to their expected C4/SIP counterparts
$unblessed->{institution} = $self->institution->name;
$unblessed->{id} = delete $unblessed->{login_id};
$unblessed->{password} = delete $unblessed->{login_password};
if ( $unblessed->{error_detect} ) {
$unblessed->{'error-detect'} = 'enabled';
delete $unblessed->{error_detect};

6
api/v1/swagger/definitions/sip2_account.yaml
View File

@ -124,10 +124,7 @@ properties:
- string
- 'null'
login_id:
description: login id
type: string
login_password:
description: login password
description: Staff userid for SIP2 authentication
type: string
lost_block_checkout:
description: lost status for missing
@ -238,4 +235,3 @@ additionalProperties: false
required:
- sip_account_id
- login_id
- login_password

7
installer/data/mysql/atomicupdate/bug_37893.pl
View File

@ -63,8 +63,7 @@ return {
`holds_get_captured` tinyint(1) NULL,
`inhouse_item_types` varchar(255) NULL,
`inhouse_patron_categories` varchar(255) NULL,
`login_id` varchar(255) NOT NULL COMMENT 'PREVIOUSLY id in Sipconfig.xml',
`login_password` varchar(255) NULL,
`login_id` varchar(255) NOT NULL COMMENT 'Staff userid for SIP2 authentication',
`lost_block_checkout` tinyint(1) NULL COMMENT 'actual tinyint, not boolean',
`lost_block_checkout_value` tinyint(1) NULL COMMENT 'actual tinyint, not boolean',
`lost_status_for_missing` tinyint(1) NULL COMMENT 'actual tinyint, not boolean',
@ -290,7 +289,7 @@ return {
next unless $sip_institution;
my $insert_accounts = $dbh->prepare(
q{INSERT IGNORE INTO sip_accounts (sip_institution_id, ae_field_template, allow_additional_materials_checkout, allow_empty_passwords, allow_fields, av_field_template, blocked_item_types, checked_in_ok, convert_nonprinting_characters, cr_item_field, ct_always_send, cv_send_00_on_success, cv_triggers_alert, da_field_template, delimiter, disallow_overpayment, encoding, error_detect, format_due_date, hide_fields, holds_block_checkin, holds_get_captured, inhouse_item_types, inhouse_patron_categories, login_id, login_password, lost_block_checkout, lost_block_checkout_value, lost_status_for_missing, overdues_block_checkout, payment_type_writeoff, prevcheckout_block_checkout, register_id, seen_on_item_information, send_patron_home_library_in_af, show_checkin_message, show_outstanding_amount, terminator) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?,?, ?, ?, ?, ?, ?, ?, ?,?,?,?,?, ?,?,?,?,?,?,?, ?,?,?,?,?)}
q{INSERT IGNORE INTO sip_accounts (sip_institution_id, ae_field_template, allow_additional_materials_checkout, allow_empty_passwords, allow_fields, av_field_template, blocked_item_types, checked_in_ok, convert_nonprinting_characters, cr_item_field, ct_always_send, cv_send_00_on_success, cv_triggers_alert, da_field_template, delimiter, disallow_overpayment, encoding, error_detect, format_due_date, hide_fields, holds_block_checkin, holds_get_captured, inhouse_item_types, inhouse_patron_categories, login_id, lost_block_checkout, lost_block_checkout_value, lost_status_for_missing, overdues_block_checkout, payment_type_writeoff, prevcheckout_block_checkout, register_id, seen_on_item_information, send_patron_home_library_in_af, show_checkin_message, show_outstanding_amount, terminator) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?,?, ?, ?, ?, ?, ?, ?, ?,?,?,?, ?,?,?,?,?,?,?, ?,?,?,?,?)}
);
my $sip_institution_id = $sip_institution->get_column('sip_institution_id');
@ -325,7 +324,6 @@ return {
my $inhouse_item_types = $SIPconfig->{accounts}->{$account_key}->{inhouse_item_types};
my $inhouse_patron_categories = $SIPconfig->{accounts}->{$account_key}->{inhouse_patron_categories};
my $login_id = $SIPconfig->{accounts}->{$account_key}->{id};
my $login_password = $SIPconfig->{accounts}->{$account_key}->{password};
my $lost_block_checkout = $SIPconfig->{accounts}->{$account_key}->{lost_block_checkout};
my $lost_block_checkout_value = $SIPconfig->{accounts}->{$account_key}->{lost_block_checkout_value};
my $lost_status_for_missing = $SIPconfig->{accounts}->{$account_key}->{lost_status_for_missing};
@ -366,7 +364,6 @@ return {
$inhouse_item_types,
$inhouse_patron_categories,
$login_id,
$login_password,
$lost_block_checkout,
$lost_block_checkout_value,
$lost_status_for_missing,

3
installer/data/mysql/kohastructure.sql
View File

@ -6107,8 +6107,7 @@ CREATE TABLE `sip_accounts` (
`holds_get_captured` tinyint(1) NULL,
`inhouse_item_types` varchar(255) NULL,
`inhouse_patron_categories` varchar(255) NULL,
`login_id` varchar(255) NOT NULL COMMENT 'PREVIOUSLY id in Sipconfig.xml',
`login_password` varchar(255) NULL,
`login_id` varchar(255) NOT NULL COMMENT 'Staff userid for SIP2 authentication',
`lost_block_checkout` tinyint(1) NULL COMMENT 'actual tinyint, not boolean',
`lost_block_checkout_value` tinyint(1) NULL COMMENT 'actual tinyint, not boolean',
`lost_status_for_missing` tinyint(1) NULL COMMENT 'actual tinyint, not boolean',

10
koha-tmpl/intranet-tmpl/prog/js/vue/components/SIP2/SIP2AccountResource.vue
View File

@ -109,17 +109,9 @@ export default {
name: "login_id",
required: true,
type: "text",
label: __("Account login id"),
label: __("Staff userid for SIP2 authentication"),
group: "Details",
},
{
name: "login_password",
required: true,
type: "text",
label: __("Account login password"),
group: "Details",
hideIn: ["List"],
},
{
name: "sip_institution_id",
type: "relationshipSelect",

6
t/cypress/integration/SIP2/Accounts.ts
View File

@ -48,7 +48,6 @@ function get_account() {
timeout: 100,
},
login_id: "asdas",
login_password: "dasdasd",
lost_block_checkout: null,
lost_block_checkout_value: null,
lost_status_for_missing: null,
@ -142,7 +141,6 @@ describe("Accounts", () => {
3
);
cy.get("#login_id").type(account.login_id);
cy.get("#login_password").type(account.login_password);
cy.wait("@getSIP2Institutions");
cy.get("#sip_institution_id .vs__search").type(
@ -284,10 +282,6 @@ describe("Accounts", () => {
// Form has been correctly filled in
cy.get("#login_id").should("have.value", accounts[0].login_id);
cy.get("#login_password").should(
"have.value",
accounts[0].login_password
);
// cy.get("#checkin_yes").should("be.checked");
// cy.get("#checkout_yes").should("be.checked");

5
t/db_dependent/Koha/SIP2/SIP2ModuleMigration.t
View File

@ -66,6 +66,10 @@ subtest 'Config from XML matches config from database' => sub {
{
delete $fileSIPconfig->{accounts}->{$key}{'register_id'};
}
# Remove 'password' from file config as it's no longer migrated (security improvement)
# Passwords are now validated against the borrowers table using bcrypt hashes
delete $fileSIPconfig->{accounts}->{$key}{'password'};
}
#Test accounts
@ -124,7 +128,6 @@ subtest 'config_timestamp is updated when database configuration changes' => sub
class => 'Koha::SIP2::Accounts',
value => {
login_id => $seen_patron->userid,
login_password => '123456',
sip_institution_id => $sip_institution->sip_institution_id,
allow_fields => '',
hide_fields => '',

9
t/db_dependent/api/v1/sip2_accounts.t
View File

@ -165,7 +165,7 @@ subtest 'get() tests' => sub {
subtest 'add() tests' => sub {
plan tests => 17;
plan tests => 16;
$schema->storage->txn_begin;
@ -192,7 +192,6 @@ subtest 'add() tests' => sub {
my $account = {
login_id => "koha",
login_password => "koha",
sip_institution_id => $account_obj->sip_institution_id,
};
@ -203,7 +202,6 @@ subtest 'add() tests' => sub {
my $account_with_invalid_field = {
blah => "Account Blah",
login_id => "koha",
login_password => "koha",
sip_institution_id => $account->{sip_institution_id},
};
@ -223,7 +221,7 @@ subtest 'add() tests' => sub {
->header_like(
Location => qr|^/api/v1/sip2/accounts/\d*|,
'REST3.4.1'
)->json_is( '/login_id' => $account->{login_id} )->json_is( '/login_password' => $account->{login_password} )
)->json_is( '/login_id' => $account->{login_id} )
->json_is( '/sip_institution_id' => $account->{sip_institution_id} )->tx->res->json->{sip_account_id};
# Authorized attempt to create with null id
@ -281,7 +279,6 @@ subtest 'update() tests' => sub {
# Attempt partial update on a PUT
my $account_with_missing_field = {
login_password => "koha",
sip_institution_id => $account->sip_institution_id,
};
@ -291,7 +288,6 @@ subtest 'update() tests' => sub {
# Full object update on PUT
my $account_with_updated_field = {
login_id => "new koha",
login_password => "koha",
sip_institution_id => $account->sip_institution_id,
};
@ -302,7 +298,6 @@ subtest 'update() tests' => sub {
my $account_with_invalid_field = {
blah => "Account Blah",
login_id => "koha",
login_password => "koha",
sip_institution_id => $account->sip_institution_id,
};