moov-io / pinblock

Personal Identification Number (PIN) management and security in financial services.
Apache License 2.0
6 stars 2 forks source link

Create describe function for encoding an decoding #2

Closed wadearnold closed 1 year ago

wadearnold commented 1 year ago

Like ISO8583 add the describe() function to create a human-readable output of the encoding inputs and outputs.

PIN blocks: PIN block encrypt operation finished
****************************************
PAN:            43219876543210987
PIN:            1234
PAD:            N/A
Format:         Format 0 (ISO-0)
—————————————-
Clear PIN block:0412AC89ABCDEF67
PIN blocks: PIN block decode operation finished
****************************************
PIN block:      0412AC89ABCDEF67
PAN:            43219876543210987
PAD:            N/A
Format:         Format 0 (ISO-0)
—————————————-
Decoded PIN:    1234

Support different formats of the pinblock

PIN blocks: PIN block encrypt operation finished
****************************************
PAN:            432198765432109870
PIN:            1234
PAD:            N/A
Format:         Format 4 (ISO-4)
—————————————-
Clear PIN block:441234AAAAAAAAAA911B9B36BC7CE94E
Clear PAN block:64321987654321098700000000000000
PIN blocks: PIN block decode operation finished
****************************************
PIN block:      441234AAAAAAAAAA911B9B36BC7CE94E
PAN block:      64321987654321098700000000000000
PAD:            N/A
Format:         Format 4 (ISO-4)
—————————————-
Decoded PIN:    1234
Decoded PAN:    432198765432109870
mfdeveloper508 commented 1 year ago

PR

alovak commented 1 year ago

Not sure how displaying PIN, PAN and output can be useful.

I see how pin and pan fields (that we calculate with the control char, length, padding, etc.) can be useful when you debug things e.g.:

PIN block:       441234AAAAAAAAAA911B9B36BC7CE94E
PAN block:      64321987654321098700000000000000

but in the #6 , we are not showing it. We are showing only two new data points into addition to what user already has: formatted name and padding.

What is the purpose of such output? I used examples from here to check if I'm doing it right. So, if our goal it to help people debug things, then I see two options for the implementation here:

Option 1 - use io.Writer as a debug output

By default, DebugOutput should be io.Discard, it can be set via constructor or by setting the field on the struct like this:

format := &formats.ISO0{
        DebugOutput: os.Stdout,
}

internally it can write any information that can be used for debugging - which can really be useful

My concern here is the security. It would be pretty bad if someone somehow will output real pins/pans somewhere without the intention to do so.

Option 2 - create methods that will show debug information so we can use them in Describe

It's similar to what we have in the #6 but we should agree on what exactly we want to see and how useful it is

For example, for iso4 we have many intermediate calculations, pin field, encrypted pin field, pan field, xor'ed encrypted pin and pan, encrypted pan (result). Which of them do we really want to show?

Here are two suboptions:

1

we can keep methods private, and move Describe into formats package so it will have access to them

2

agree on the Describable interface and use it instead of adding interface for the format (ISO) (because: define interface where you use it, not where you implement it).

alovak commented 1 year ago

@adamdecaf what do you think about Option 1? Do you think it's ok to allow setting DebugOutput?

adamdecaf commented 1 year ago

I like Option 1 and the default io.Writer can be nil so that we only write when .DebitWriter != nil. Writing all of that unused debug information isn't great from a privacy/PCI point of view as those strings will reside in memory for a period of time.

mfdeveloper508 commented 1 year ago

option 1 is good for me too

alovak commented 1 year ago

Let's think about the API for this. I have following suggestions

Option 1 - separate method to set debug writer + without exporting the field

type ISO0 struct {
    Filler      string
    debugOutput io.Writer
}

/// usage
pinblock := &formats.NewISO0()
pinblock.SetDebugWriter(os.Stdout)

as I have a security concern, the first option seems to be a better choice. When the engineer explicitly calls a method like SetDebugWriter(), it's clear that they're enabling debug logging, which should make them more aware of what they're doing. Also, debugOutput is unexported, which means it cannot be directly accessed outside the package.

Option 2 - just set the field

type ISO0 struct {
    Filler      string
    DebugOutput io.Writer
}

// usage
pinblock := &formats.NewISO0()
pinblock.DebugOutput = os.Stdout

it's more straightforward, but I would prefer the Option 1

@mfdeveloper508 @adamdecaf what is your opinion?

adamdecaf commented 1 year ago

Go usually avoids setters/getters. If someone calls pinblock.DebugOutput = os.Stdout they know what they are doing. To me that's the same as calling .SetDebugOutput(..).

Option 2 is more the Go standard way of writing this.